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

Get --debug flag from sys.argv instead of using an astropy helpers routine #9731

Merged
merged 1 commit into from
Dec 18, 2019

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 presence of the --debug flag to setup.py was used to control whether certain warnings should be shown or not when compiling the io.fits and wcs extensions. For similar reasons to #9730 it makes more sense to switch to using an environment variable to control this. Note that this is not a documented feature, hence why there are no changes to the docs.

I'm not super happy with the variable name so far, so am open to other suggestions - @mhvk?

EDIT: changed to getting --debug from sys.argv

@astrofrog astrofrog added Refactoring 💤 astropy-helpers-removal PRs and issues related to the removal of astropy-helpers labels Dec 6, 2019
@astrofrog astrofrog added this to the v4.1 milestone Dec 6, 2019
@pllim
Copy link
Member

pllim commented Dec 6, 2019

Env var is no fun on Windows, but I guess it is a small price to pay for simplification, so 👍

@mhvk
Copy link
Contributor

mhvk commented Dec 6, 2019

Maybe ASTROPY_SHOW_ALL_COMPILATION_WARNINGS (or maybe with _ALL?)

Changes look good otherwise.

@astrofrog
Copy link
Member Author

If we want we could actually add back command-line flags in setup.py that sets the env var automatically, but maybe let's not do that first and do it if anyone needs it?

@mhvk
Copy link
Contributor

mhvk commented Dec 6, 2019

I had been wondering about being able to set defaults in setup.cfg, but felt one should do it only if there is some standard, commonly accepted approach. For now, probably best to just make the simplification;

@astrofrog
Copy link
Member Author

@mhvk - I like your suggestion for the variable name but then I thought maybe ASTROPY_VERBOSE_COMPILATION_WARNINGS could work too and require fewer words. What do you think?

mhvk
mhvk previously approved these changes Dec 7, 2019
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.

I like the name, and it all looks good.

@saimn
Copy link
Contributor

saimn commented Dec 10, 2019

Not sure about this one: --debug is not some custom option from astropy, it is a setuptools thing. And I think it makes sense to not hide these warnings when compiling in debug mode with this setuptools option, without having to know that these is another env var to activate.

@mhvk
Copy link
Contributor

mhvk commented Dec 10, 2019

Ah, so --debug actually does do a lot more!? If we do not need anything from astropy-helpers to use it, then I agree it makes more sense to just stick with what we have.

@astrofrog
Copy link
Member Author

Yes indeed --debug is a setuptools option but I can't find a clean way to find out if that option is present without having to have some hackery as currently done in astropy-helpers. If there is a standardized way to get this option, then I'm all for it, but otherwise I think we have to find a different way as done here. I suppose we could imagine also just checking for the presence of the --debug flag in sys.argv in setup_package.py files?

@mhvk mhvk dismissed their stale review December 10, 2019 18:51

Continuing discussion about approach

@mhvk
Copy link
Contributor

mhvk commented Dec 10, 2019

I wasted the better part of an hour only to come to your conclusion, that the --debug option is not much exposed at all. I think for this corner case I'd be quite happy with something like
if '-g' in sys.argv or '--debug' in sys.argv

p.s. As far as I can see, --debug does actually rather little, mostly just compiling in a different directory and adding a -g flag (which is there by default anyway...)

@astrofrog
Copy link
Member Author

Ok I've updated this to look for --debug in sys.argv. I didn't check for -g since it may be there by default in some cases and I think we really should just check for an explicit --debug.

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.

Super, let's go this way!

@mhvk
Copy link
Contributor

mhvk commented Dec 18, 2019

The failure seems unrelated but real. Maybe a rebase is needed?

@mhvk mhvk changed the title Use environment variable instead of --debug flag to control extension warnings Get --debug flag from sys.argv instead of using an astropy helpers routine Dec 18, 2019
@astrofrog
Copy link
Member Author

This is already rebased, very confused by the failure...

@astrofrog
Copy link
Member Author

It's a race condition because two doctests are using the filename values.dat (the 32-bit build is also the parallel build). I think we should not include that fix here though since that will likely need to be backported. I'll open a separate PR.

@astrofrog
Copy link
Member Author

astrofrog commented Dec 18, 2019

See #9800 for the (unrelated) 32-bit failure.

@astrofrog
Copy link
Member Author

astrofrog commented Dec 18, 2019

I'm going to go ahead and merge this since it was approved and the 32-bit failure is unrelated - and APE 17 has been merged.

@astrofrog astrofrog merged commit 5d70b20 into astropy:master Dec 18, 2019
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