Repository navigation
Add a --use-system-expat option to use the system's expat library. - #316
Conversation
There was a problem hiding this comment.
It says "get_extra_build_options" here, but in xml/setup_package.py you have get_build_options. Is one of these a typo?
There was a problem hiding this comment.
Indeed. I think the "extra" is a typo -- sort of redundant. I'll fix this in the docs.
|
Hmm... I'm a little hesitant on this one, because we definitely want to implement a general way of specifying whether or not to use system libraries. (This may or may not be connected into the (also to-be-implemented) python-level optional dependencies.) If we do that, this option might be confusing because it might conflict with that framework (unless the "--use-system-`` scheme is what we use for that). The |
|
Even if we provide a general way to "use no internally shipped libraries", which perhaps is useful for end users, but not terribly useful for packagers, we still need a way to specify each individually. Imagine a scenario (not too far from the truth) where Debian ships an old version of wcslib that won't work with astropy, but its expat is compatible. The packager will want to specify using one and not the other. I think it's simple enough to add another build option "use-system-libraries", and change the check from to How these flags are ultimately handled is necessarily library-specific anyway, so I don't know how much further one could generalize this. |
There was a problem hiding this comment.
I like this, though the way it updates locals() at the end seems unnecessarily hackish. Could it not just return maybe a dict and let the caller decide what it wants to do with the results? This works for expat, but if there isn't already a local variable called library_dirs referencing a list this will fail.
I'd rather just do something like:
flag_updates = pkg_config(...)
if 'library_dirs' in flag_updates:
library_dirs.extend(flag_updates['library_dirs'])
(maybe the if statement could be skipped if the returned dict is guaranteed to contain certain keywords, which may just have empty lists as their values).
There was a problem hiding this comment.
The idea was that this function will be used in one context: to add additional elements to the include_dirs, library_dirs and libraries kwargs of a call to distutils.Extension. Requiring all of the callers to do that work, which admittedly amounts to three lines of code appending lists together, just seemed like unnecessary duplication.
But I do think, given your question, that it could be made more obvious what it is doing. For example, there will always be a local variable named library_dirs, because it is an argument to the function -- and yes, it must be a list, but that's mentioned in the docstring. Maybe having an if statement in the loop rather than using a mapping dictionary to sort the arguments into categories would be more obvious?
|
Oh, I agree the actual implementation of what to do if someone wants the system version has to be specified - I'm more thinking of putting in infrastructure to make it clear for every package how to say "always use the system version of library x". The sort of scenario I'm thinking of is that some future subpackage might also want to use expat, but they might add a build option "--use-local-expat" or something like that. We don't want users (or packagers) to have to sort through that, so instead we should include a standard way of looking for which libraries should use external vs. bundled versions. Ideally, this is the sort of thing we could tell people to add to their This is already pretty close to what you have implemented, though... So could you use a Does that sound reasonable? |
|
@eteq: I've added another commit that I think addresses your concerns. There are two new functions I don't like the idea of having a single option |
|
@mdboom - yep, with these changes it looks perfect! You might add a brief section in Other than that, it looks set to merge to me. |
…guments are listed as being from the "build" command in the help.
There was a problem hiding this comment.
Maybe throw in a sentence at the end here that says something like "you can also put use-system-expat=1 in setup.cfg to make this the default behavior." Or not, if you think that makes it too confusing.
|
Feel free to incorporate that one minor comment (or not if you prefer) and merge! |
Add a `--use-system-expat` option to use the system's expat library.
Add a `--use-system-expat` option to use the system's expat library.
ENH: New logo
This makes is easier for packagers to specify that they want to use the system's expat library.
It also lays the groundwork for doing this more generally, namely:
Easily adding build arguments from a packages
setup_package.pyMaking pkg-config calls, if necessary and available, to get the compiler arguments to link to a particular library.
@iguananaut may want to look at this since
io.fitsalso has some external packages that may need similar functionality.