-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
dependencies: Add command line option for pkg_config_path #4931
Conversation
Like all PRs that add options, this puts pressure to fix #4574 somehow. |
Shouldn't this go into the native file first? |
It sounds like this is Mesa's last requirement before we can delete autotools. |
How does this work together with cross compilation? FWICT this should only be used for native lookup but not for cross lookup (because it uses a prefixed pkgconfig that should have all of these things already defined). |
For my cross compilation I still want to be able to set pkg_config_path to my personal install prefix (just like I do for non-cross compiles), not just what the cross tool provides. |
Sounds like we want to namespace this command-line option separately for native and cross builds, and also allow it to be set in the native and cross files. |
I'm becoming convinced that we really want our arguments to target the host rather than the build target. I generally don't care if some tool I need to build and use as part of my build process is built with some set of args but I really do care about that for the host, wether host == build or not. I know that's a behavioral change and probably deserves a separate discussion, but I feel like we keep coming back to this. |
bf36ae1
to
6451f80
Compare
We've added the 'cross_' prefix as a reserved prefix now, do I need to implement support for passing |
Currently the builtins are stored as lists, then there are a mess of functions which take said lists, check what the type of the first thing is, and then extract data from various points in the list based on that. This is stupid. This is what structs/classes are for.
This returns a list out of th keys of a dict. In both cases of use remaining though it's used for checking membership, checking for list membership, lists are O(n) lookup, while dicts are O(1), so removing the abstraction reduces typing and improves performance.
This is a silly abstraction at this point.
Since the attribute is always accessible from a BuiltinOptions instance we don't need a special function for this.
We can remove some guards here since we know the object will have certain attributes.
It really is a method of the BuiltinOption class, not a standalone function.
6451f80
to
54439d4
Compare
Alright, I've added the cross_pkg_config_path argument (a bunch of cleanup was required to get there), hopefully this is closer to the form people were wanted. |
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.
Overall, I like this and it's good cleanup too. One thing I'm increasingly concerned about is that user options and built-in options share the same namespace, which means we increasingly have a chance to break someone's build files when we add new ones.
It's something we should think about, but it's not related to this PR of course and shouldn't block it.
87187e9
to
937a52d
Compare
Since we're adding arguments that use the cross_ prefix but are valid we don't want to print "warning invalid argument name!", as that will confuse people by virtue of being wrong.
937a52d
to
60cc039
Compare
I don't understand why the cross tests are failing to run, but I've added a couple extra patches to silence the warnings about "cross_" prefix used when that prefix is part of a builtin option. |
I don't konw how the heck this has ever worked, but you know.
This creates a new command line option to store pkg_config_path into, and store the environment variable into that option. Currently this works like the environment variable, for both cross and native targets.
Currently this only marks the pkg_config_path flag, but it could be used to mark any of these values as having separate values for cross and native builds, assuming that the necessary plumbing is done.
60cc039
to
91b2475
Compare
Good change!
Yes indeed! Let's finish this off in #4963 ? There's a slew of interface changes my distro desperately wants depending on everyone reaching an understanding here. |
@dcbaker , as far as I understand, this PR added support for |
It added the infrastructure for it, but it did not enable it. See: 377b652 |
'optimization': BuiltinOption(UserComboOption, 'Optimization level', '0', choices=['0', 'g', '1', '2', '3', 's']), | ||
'debug': BuiltinOption(UserBooleanOption, 'Debug', True), | ||
'wrap_mode': BuiltinOption(UserComboOption, 'Wrap mode', 'default', choices=['default', 'nofallback', 'nodownload', 'forcefallback']), | ||
'pkg_config_path': BuiltinOption(UserArrayOption, 'List of additional paths for pkg-config to search', [], separate_cross=True), |
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.
This should be added to https://github.com/mesonbuild/meson/blob/master/docs/markdown/Builtin-options.md
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.
Yes, we should also add a unit test for it so that it doesn't happen again.
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.
@nirbheek I could write such unit test , but that will require the exposure of global builtin_options
var through some means (according to my limited knowledge it's impossible to access global vars directly from other files/scripts)
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.
See: run_unittests.py:DataTests()
, we already have a test for the compiler options. The same can be done for builtin options too.
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.
See #5233
Oh, right, sorry. I've misunderstood the meaning of |
FYI #5263 would give us |
This creates a new command line option to store pkg_config_path into,
and store the environment variable into that option. Currently this
works like the environment variable, for both cross and native targets.