Skip to content

Use environment variables to select system libraries#9730

Merged
astrofrog merged 3 commits intoastropy:masterfrom
astrofrog:system-libraries-env-var
Dec 19, 2019
Merged

Use environment variables to select system libraries#9730
astrofrog merged 3 commits intoastropy:masterfrom
astrofrog:system-libraries-env-var

Conversation

@astrofrog
Copy link
Member

@astrofrog astrofrog commented Dec 6, 2019

This is a self-contained subset of changes related to the APE A roadmap for package infrastructure without astropy-helpers. This PR as-is is ready for review, and does not actually require removing astropy-helpers to work, though I think we should maybe wait until Tuesday to merge since that's when we'll be discussing the APE in Socorro.

Previously, the --use-system-... command-line options required patching the setup.py commands, and these were also not easy to set when doing pip installs. The environment variable approach no longer requires patching the commands, and works regardless of whether pip or setup.py is used. This approach is described in the APE.

I checked with @olebole who confirmed this approach would not pose issues for Debian packaging.

Copy link
Contributor

@mhvk mhvk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks all OK except that I would really like those very long lines to be split... But that's nitpicky enough that I'll approve.

@astrofrog
Copy link
Member Author

@mhvk - I split the lines in my latest commit

@astrofrog astrofrog requested review from eteq and nden December 18, 2019 13:25
@astrofrog
Copy link
Member Author

@eteq @nden - just want to make sure you are happy with the changes that touch ERFA and WCSLIB?

Previously, the --use-system-... command-line options required patching the setup.py commands, and these were also not easy to set when doing pip installs. The environment variable approach no longer requires patching the commands, and works regardless of whether pip or setup.py is used.
@astrofrog astrofrog force-pushed the system-libraries-env-var branch from 8e10279 to c0ccadf Compare December 18, 2019 14:20
@astrofrog astrofrog force-pushed the system-libraries-env-var branch from 50a2873 to 05cacd1 Compare December 18, 2019 14:35
@astrofrog
Copy link
Member Author

Since @mhvk has already approved this, @olebole is happy with this approach for Debian packaging, and given that people approved APE 17 which describes this approach, in the interest of finishing up #9726 I am going to go ahead and merge this. If absolutely needed, we can consider adding back command-line flags to setup.py by just checking sys.argv and updating os.environ, but I'd rather avoid it if possible.

@astrofrog astrofrog merged commit febda48 into astropy:master Dec 19, 2019
@MSeifert04
Copy link
Contributor

Is this something that needs a "What's new"?

@astrofrog
Copy link
Member Author

@MSeifert04 - it may only be relevant for distribution maintainers, so it might make more sense to just ping them all directly?

@MSeifert04
Copy link
Contributor

Not sure, that's why I asked. Having some sort of "if you had x before, now you need to do y" in a whatsnew could help those who had the need or urge to build against system libraries - but if that's something that only distribution maintainers used then pinging them directly makes more sense.

@astrofrog
Copy link
Member Author

I'll label in just in case, and we can decide later :)

@mhvk
Copy link
Contributor

mhvk commented Dec 19, 2019

I think a what's-new is needed, but would do it as part of the larger astropy-helpers removal (i.e., a what's new that includes a checklist of related changes)

@pllim
Copy link
Member

pllim commented Dec 19, 2019

Wait a minute... Didn't @astrofrog insist that what's new entry should be added as part of the PR instead of waiting till release time?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants