Skip to content

Add bringris to two of the build systems #137

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

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

Add bringris to two of the build systems #137

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c7ff0e8
add bringris to Makefile and mymake
still-flow Nov 2, 2020
4889f55
refactor Makefile.simple; fix one warning
still-flow Nov 2, 2020
8da810f
add Travis entries for bringris
still-flow Nov 2, 2020
ee047d2
add GH CI entries for bringris
still-flow Nov 2, 2020
12031b3
glew stuff together
still-flow Nov 2, 2020
424bb61
fix sometimes-uninitialized warnings
still-flow Nov 3, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Makefile.simple
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@Quuxplusone Quuxplusone Nov 2, 2020

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

@still-flow still-flow Nov 2, 2020

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

@Quuxplusone Quuxplusone Nov 2, 2020

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

@still-flow still-flow Nov 2, 2020

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

@zenorogue zenorogue Nov 5, 2020

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

@still-flow still-flow Nov 5, 2020

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

@Quuxplusone Quuxplusone Nov 5, 2020

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


.PHONY: clean

clean:
Expand Down
4 changes: 4 additions & 0 deletions mymake.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,10 @@ int main(int argc, char **argv) {
obj_dir += "/web";
setdir += "../";
}
else if(s == "-bringris") {
modules.push_back("rogueviz/bringris");
modules.push_back("rogueviz/subquotient");
}
else if(s.substr(0, 2) == "-f") {
opts += " " + s;
obj_dir += "/";
Expand Down