Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix compile failed without gcc #3130

Merged
merged 3 commits into from
Dec 10, 2017
Merged

Fix compile failed without gcc #3130

merged 3 commits into from
Dec 10, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 10, 2017

will fix #3129

@lunny lunny added the topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile label Dec 10, 2017
@lunny lunny added this to the 1.4.0 milestone Dec 10, 2017
@lunny lunny changed the title Fix comile failed without gcc Fix compile failed without gcc Dec 10, 2017
@codecov-io
Copy link

codecov-io commented Dec 10, 2017 •
edited
Loading

Codecov Report

Merging #3130 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3130      +/-   ##
==========================================
+ Coverage   34.83%   34.84%   +<.01%     
==========================================
  Files         274      275       +1     
  Lines       40026    40028       +2     
==========================================
+ Hits        13943    13946       +3     
+ Misses      24085    24084       -1     
  Partials     1998     1998
Impacted Files Coverage Δ
models/unit_tests.go 85% <ø> (ø) ⬆️
modules/test/context_tests.go 0% <ø> (ø) ⬆️
models/repo_indexer.go 48.54% <0%> (-0.98%) ⬇️
models/models_sqlite.go 100% <0%> (ø)
modules/process/manager.go 81.15% <0%> (+4.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eb6cbed...51ec09f. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 10, 2017
@ethantkoenig
Copy link
Member

LGTM

@tboerger tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 10, 2017
ethantkoenig
ethantkoenig suggested changes Dec 10, 2017
View reviewed changes
Copy link
Member

@ethantkoenig ethantkoenig left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I should have looked more closely before giving a LGTM

Makefile
@@ -131,7 +131,8 @@ fmt-check:

.PHONY: test
test:
$(GO) test $(PACKAGES)
$(GO) build # test if go build succeed without sqlite support
Copy link
Member

@ethantkoenig ethantkoenig Dec 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this under test? IMO, the make test target should run unit tests, nothing more.

Also, building without the sqlite tag already runs in CI (make build), so why is doing it again necessary?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make build will always add -tags=sqlite. This is a test for building without sqlite. Since all test now always use sqlite.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I see why building again, without the sqlite tag, is necessary. However, could we please not put this in the test make target, for the sake of keeping our make targets straightforward? One option would be to just run make build an additional time in CI (without the sqlite tag).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or Maybe a new section named comiple-test or build-test?

@lafriks
Copy link
Member

lafriks commented Dec 10, 2017

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 10, 2017
@appleboy
Copy link
Member

LGTM

@lafriks lafriks merged commit 41d6afe into go-gitea:master Dec 10, 2017
@lafriks
Copy link
Member

lafriks commented Dec 10, 2017

Sorry @ethantkoenig did not see that you have not approved

@ethantkoenig ethantkoenig mentioned this pull request Dec 10, 2017
Merged
@lunny lunny deleted the lunny/fix_compile_without_gcc branch December 11, 2017 01:27
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ethantkoenig ethantkoenig ethantkoenig requested changes

@appleboy appleboy appleboy approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/build PR changes how Gitea is built, i.e. regarding Docker or the Makefile
Projects
None yet
Milestone
1.4.0
Development

Successfully merging this pull request may close these issues.

Compiling without gcc
6 participants
@lunny @codecov-io @ethantkoenig @lafriks @appleboy @tboerger