-
-
Notifications
You must be signed in to change notification settings - Fork 10.9k
BUILD: Hide platform configuration probe behind --debug-configure #14518
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
Conversation
Should I add the |
@@ -16,8 +16,8 @@ class build(old_build): | |||
user_options = old_build.user_options + [ | |||
('fcompiler=', None, | |||
"specify the Fortran compiler type"), | |||
('parallel=', 'j', |
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 change seems unrelated, why does parallel
need to be removed here and in finalize_options
?
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.
It is redundant - the base build
class has this option (on python 3)
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.
Specifically it was added in CPython v3.5.0
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.
Ah yes. We may have to add it back once we figure out why it's buggy right now, but let's remove it till someone gets annoyed enough to start looking into that.
# build_clib cannot handle generate_config_h and generate_numpyconfig_h | ||
# (don't ask). Because clib are generated before extensions, we have to | ||
# explicitly add an extension which has generate_config_h and | ||
# generate_numpyconfig_h as sources *before* adding npymath. |
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.
what's going on here, did all this become obsolete somehow?
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.
It should have been removed when we merged umath
and multiarray
. It causes the config file generation to be run twice, which I noticed when working on this PR.
Probably a good idea. The |
Thanks for tackling this, will be very nice to have! |
Comparing the logs from another PR's travis run to this one, the lines from building library "npysort" sources to building library "npysort" sources to |
3c24f68
to
70dedf5
Compare
70dedf5
to
aab36ae
Compare
failing because of #14526 |
a039c5f
to
e05617f
Compare
runtests.py
Outdated
@@ -67,6 +67,8 @@ def main(argv): | |||
parser = ArgumentParser(usage=__doc__.lstrip()) | |||
parser.add_argument("--verbose", "-v", action="count", default=1, | |||
help="more verbosity") | |||
parser.add_argument("--debug-configure", action="store_true", | |||
help="add -v to build_src to show cconfiguration compiler output") |
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 is pretty vague; can we describe what the actual feature is that is enabled?
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.
"adds -v to the build_src subcommand which will show compiler output while probing platform features used to create _numpyconfig.h and config.h" seems a bit too much.
Any ideas what would be more appropriate?
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.
I think the problem is that there are several things being addressed here: -v
is being added to build_src
, but only for a certain type of build? Or is it always added? If always, then the latter part of the sentence is not necessary. It can simply be something like "Increase verbosity of build_src by adding -v flag". But the "configure" in --debug-configure
makes me think it was intended for something more specific.
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.
another option would be to not add it to runtests.py at all (which would always build without the flag), and in CI explicitly do setup.py build build_src -v; runtests.py
.
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.
and in CI explicitly do
setup.py build build_src -v; runtests.py
That doesn't seem desirable at all, we want to run CI in the ways we expect humans to use things, not some arcane invocation. There's no real issue here, so why change anything?
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, I think just a slightly more descriptive help string would be sufficient.
47e6110
to
09e01c0
Compare
CI failure I'm -1 on the interface changes and promoting |
09e01c0
to
c0c31f8
Compare
c0c31f8
to
492fdab
Compare
rebased and reverted the change. The help string now reads
clarifications happily accepted. |
That help description seems very clear to me, thanks @mattip. |
In it goes, very nice to have this! |
Hmm, one of the Windows builds on Azure for the merge commit failed, see https://dev.azure.com/numpy/numpy/_build/results?buildId=5577. Related to gh-14464, which may have a bad interaction with this PR. Any thoughts @mattip?
|
That is the infamous heisenbug that occurs 1/1000. |
This broke the appveyor wheel builds. See https://ci.appveyor.com/project/matthew-brett/numpy-wheels/builds/27571654/job/v5tm7ljl6htcfa0a. Probably the changes in |
BLD, DOC: fix gh-14518, add release note
As requested in #14470, this adds a
--debug-configure
option toruntests.py
andsetup.py build
to hide all the compiler output during the configuration probe to createconfig.h
and_numpyconfig.h
.This also converts some of the direct
print
to stdout so that it uses the logger instead. The lack of the flag simply raises the log level toWARN
fromINFO
for the duration of the noisy function.