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

Compile flag order mixup #74

Closed
NOhs opened this issue Aug 14, 2019 · 9 comments · Fixed by #76
Closed

Compile flag order mixup #74

NOhs opened this issue Aug 14, 2019 · 9 comments · Fixed by #76
Labels
bug Something isn't working

Comments

@NOhs
Copy link
Collaborator

NOhs commented Aug 14, 2019

I am not sure if this has been an issue so far, but I just noticed that in target.py the compile flags order is scrambled. I think the intent was to get rid of duplicates, but the current implementation could change the order:

self.compile_flags = list(set(self.compile_flags))

@NOhs NOhs added the bug Something isn't working label Aug 14, 2019
@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

I think one solution is to not actually remove duplicates. Clang can handle duplicates, as long as the order is correct, this should be fine.

@GPMueller
Copy link
Contributor

However, duplicate flags can make the commands quite unreadable.
Doesn't Python keep the first entry in a list of any item that appears twice? In that case things should be fine as long as the order of appending (e.g. flags from dependencies versus own) is correct.

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

I think the issue is that set does not make any guarantees about order. So when the set is created, everything is thrown around. See also:

https://stackoverflow.com/questions/26420945/python-order-of-elements-in-set

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

This might also be an issue for veeeery ugly projects where the order of the include folders matter, because that order is also mangled (see line 85):

self.headers = list(set(self.headers))

@GPMueller
Copy link
Contributor

I believe I had errors when the -std= flag appeared more than once, even if it was the same standard both times.
Currently not sure what else it might have been, but I believe certain flags should not appear multiple times in different positions.

In the case of link flags especially, it is of course important to keep the order. Maybe a self-written function for eliminating duplicates would do best?

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 14, 2019

I think we can just switch to dict as discussed here: https://stackoverflow.com/a/7961390/2305545

@NOhs
Copy link
Collaborator Author

NOhs commented Aug 21, 2019

I think IF we keep this, we should actually keep the last occurence of a keyword and not the first, otherwise we have the issue again, that if we have the dependency

my_proj   <-   lib   <- lib2

And lib2 defines the same flag as my_proj, but lib defines overriding flags, lib would "win".

@GPMueller
Copy link
Contributor

I agree, the toplevel target flags should override those of its dependencies.

@GPMueller
Copy link
Contributor

If we preserve a specific order of flags etc. in inheritance, this order should be documented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants