-
-
Notifications
You must be signed in to change notification settings - Fork 73
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.
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
Add bringris to two of the build systems #137
base: master
Are you sure you want to change the base?
Conversation
fbd8a30
to
a0b21ab
Compare
- add bringris entry to Makefile.simple - add -bringris option to mymake
a0b21ab
to
c7ff0e8
Compare
Makefile.simple
Outdated
@@ -171,6 +171,9 @@ emscripten: hyper.html | |||
|
|||
hyper.emscripten-sources: *.cpp autohdr.h | |||
|
|||
bringris$(EXE_EXTENSION): autohdr.h | |||
$(CXX) $(CXXFLAGS) -Wno-error -DBRINGRIS rogueviz/bringris.cpp $(LDFLAGS) $(hyper_LDFLAGS) -o $@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd vastly prefer to see -Wno-whatever-specific-warning
, not a blanket -Wno-error
.
Also probably this should be split into two rules,
bringris.o: bringris.cpp autohdr.h
$(CXX) $(CXXFLAGS) $(bringris_CXXFLAGS) bringris.cpp -c -o $@
bringris$(EXE_EXTENSION): bringris.o
$(CXX) $(CXXFLAGS) $(bringris_CXXFLAGS) bringris.o $(LDFLAGS) $(bringris_LDFLAGS) -o $@
Also, "if you liked it then you should have put a test on it" — please add it to the TravisCI config as part of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re CI entry: I was just thinking about it. How should I organize it, though? Will just adding one Linux and one MacOS entry suffice (...and Windows. Oh.)?
Also, it's probably best done in a later PR, since I'd like this one to be reviewed by zenorogue first. So that I'm sure this is the Official way of building things. Especially when it comes to Windows, since, y'know, Windows.
Re splitting of the rule: yeah, it's likely better to do this. At the very least for convenience of re-building when source changes.
As to -Wno-error
, I should remove it altogether. It's just that then there's the burden of fixing those warnings, but I guess I shouldn't be so lazy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, it's probably best done in a later PR, since I'd like this one to be reviewed by zenorogue first
My strong feeling is that it's best to put it all in one PR (and in fact in one commit). That way, you get the whole thing reviewed by zenorogue, instead of having to come back later and say "oh yeah here are the tests," which incidentally risks having the code committed for a long time without the tests, which is how you get bit-rot. (I say this for all changes, in all repos; this PR isn't special in any way AFAIC.)
Re -Wno-error
and fixing warnings: The important thing is to hold the bridge against new warnings/bugs. So I don't object at all to committing a laundry-list like -Wno-unused-parameter -Wno-implicit-fallthrough -Wno-maybe-uninitialized
. That's just, like, a TODO list. It's not a bad thing. Subsequent commits can deal with the TODO list's items one by one (e.g. 41669ab, fbc7cd3), and meanwhile any bug that doesn't fit one of the shapes on the laundry-list is caught by CI. The problem with -Wno-error
is that it permits an open/unbounded set of bugs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, that makes sense. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Talking about -Werror
, one thing I do not like about it is that it may cause problems for users if they are using a new version of gcc which introduces some new warning. For example, "maybe-uninitialized" and "misleading indentation" were introduced at some point, and they sometimes show up even if there is no bug. Good for CI and developers, but not sure about normal users who may not know how to fix the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-Werror may cause problems for users if they are using a new version of gcc which introduces some new warning
That certainly is a problem. I think fixing it shouldn't be too difficult: something like gutting currently hardcoded switches in Makefile.simple
and mymake
, and then updating CI configs to force the flag anyway. Well, it's probably not that simple, but I can give it a try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, "there is a warning with some major compiler's -Wall -Wextra
" and "there is a bug in the code" should be treated as synonymous. Code should compile clean. Once the programmer finds out about a warning, it's easy to fix it in the code.
So the concern here seems limited to "what happens if some non-developer end-user [unable to easily edit Makefiles] is trying to compile HyperRogue from source, and their compiler is newer than anything we run in CI, such that it catches a new bug/warning we weren't aware of?" Personally I don't believe that's a likely situation — especially with all the effort @still-flow seems to be pouring into expanding the number of platforms that we CI on. :)
I think it would be relatively easy to move -Werror
out of Makefile.simple
and into .travis.yml
; but IMO it would not bring any real benefit.
(My impression is that this subthread is essentially unrelated to this specific PR.)
Add
bringris
entry toMakefile.simple
andmymake
. Had to squeeze-Wno-error
into theMakefile.simple
entry for now, because reasons.