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.

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

still-flow
Copy link
Contributor

@still-flow still-flow commented Nov 2, 2020

Add bringris entry to Makefile.simple and mymake. Had to squeeze -Wno-error into the Makefile.simple entry for now, because reasons.

@still-flow still-flow marked this pull request as draft November 2, 2020 16:04
@still-flow still-flow changed the title Add bringris entry to Makefile.simple Add bringris to two of the build systems Nov 2, 2020
@still-flow still-flow marked this pull request as ready for review November 2, 2020 16:19
mymake.cpp Outdated Show resolved Hide resolved
- add bringris entry to Makefile.simple
- add -bringris option to mymake
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 $@
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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!

Copy link
Owner

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.)

@still-flow still-flow marked this pull request as draft November 2, 2020 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants