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

Potential bug in flags order #81

Closed
NOhs opened this issue Aug 14, 2019 · 3 comments · Fixed by #86
Closed

Potential bug in flags order #81

NOhs opened this issue Aug 14, 2019 · 3 comments · Fixed by #86
Labels
discussion Something that might end up being labled enhancement or wontfix

Comments

@NOhs
Copy link
Collaborator

NOhs commented Aug 14, 2019

for target in self.dependency_targets:
# Header only libraries will forward all non-private flags
if self.__class__ is HeaderOnly:
# Interface
self.compile_flags_interface += target.compile_flags_interface
self.link_flags_interface += target.link_flags_interface
# Public
self.compile_flags_public += target.compile_flags_public
self.link_flags_public += target.link_flags_public
# Static libraries will forward interface flags and apply public flags
elif self.__class__ is StaticLibrary:
# Interface
self.compile_flags_interface += target.compile_flags_interface
self.link_flags_interface += target.link_flags_interface
# Public
self.compile_flags += target.compile_flags_public
self.link_flags += target.link_flags_public
# Shared libraries and executables will not forward flags
else:
# Interface
self.compile_flags += target.compile_flags_interface
self.link_flags += target.link_flags_interface
# Public
self.compile_flags += target.compile_flags_public
self.link_flags += target.link_flags_public
self.compile_flags = list(set(self.compile_flags))
# Interface flags
cf, lf = self.parse_flags_options(options, 'interface-flags')
self.compile_flags_interface += cf
self.link_flags_interface += lf
# Public flags
cf, lf = self.parse_flags_options(options, 'public-flags')
self.compile_flags += cf
self.link_flags += lf
self.compile_flags_public += cf
self.link_flags_public += lf

As you can see in the code above, the target's general flags are before the dependency flags, whereas the public flags are after the dependency flags. I mean these are all ugly scenarios where this would matter, but I think the current solution is very un-intuitive. We should decide which one comes first, and then do it for all (private/public/interface).

@NOhs NOhs added the discussion Something that might end up being labled enhancement or wontfix label Aug 14, 2019
@GPMueller
Copy link
Contributor

I agree. The interface and public flags of the target should simply be moved in front of the dependency flags and that would do it, right?

Or are you suggesting e.g. the public flags (target + dependency) come first, then the interface (again target + dependency) etc.?

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 15, 2019

I think the dependency flags should be moved in front of the target flags. Not having the option to override dependency flags in the current target would be very annoying. E.g. the dependency says std14 but you want std17.

@GPMueller
Copy link
Contributor

I agree, as long as in such cases the last flag is indeed the one that is used and this doesn't instead result in an error. As far as I can tell this is the case.

Note if the above does not hold: for flags that definitely don't collide (e.g. include directories, -I ...) I believe it is nicer to read if the target's own flags come first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Something that might end up being labled enhancement or wontfix
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants