-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Generate legacy metadata in temporary directory #7978
Conversation
ced9040
to
2353140
Compare
Editable installs need the egg-info to be in the location that it's in for working properly IIRC. Other than that, this all sounds and looks good to me in principle. |
Sure, but does pip need to have |
I'm pretty sure that setuptools doesn't see that, so it should be fine to put that in a temporary directory. |
Before it was generated in a pip-egg-info subdirectory of the source dir. This will avoid polluting source dir when we build in place.
6e59b69
to
fbc3c97
Compare
Ok, I added a commit with the full simplification of |
def _find_egg_info(source_directory, is_editable): | ||
# type: (str, bool) -> str | ||
"""Find an .egg-info in `source_directory`, based on `is_editable`. | ||
def _find_egg_info(source_directory): |
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.
def _find_egg_info(source_directory): | |
def _find_egg_info(directory): |
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.
The function body and docstring also need to match this change :)
Always generate legacy metadata in a temporary directory, even in the editable case. Generating it in the source directory did not add value, because setup.py develop always regenerates the .egg-info directory.
fbc3c97
to
030578e
Compare
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.
Nice simplification 👍
Thanks for the review @xavfernandez |
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.
These simplifications make me feel happy about factoring this code out of the depths of InstallRequirement
+ friends. :)
@RonnyPfannschmidt this is the PR that removed |
When neither the scm nor a pkg-info file are part of the external source tree, setuptools_scm has no way to access the version information |
@RonnyPfannschmidt yep, I know the case with setup.py in subdirectory, but in my experience it never worked with |
I don't have a reproducer, i observed this breakage before your pr, and AFAIK out of tree builds are coming back due to issue |
Ok, then if it's unrelated to this PR, the problem you mentioned on discuss is not new, and is part of the known issues with out of tree builds, as summarized in #7555. |
Progresses #7555 by getting rid of
pip-egg-info
as mentioned in #7882 (comment).Pip generates legacy metadata by calling
setup.py egg_info
. For editables, it does that in the source directory, else it does it in apip-egg-info
subdirectory of source dir.To avoid polluting source dir with this
pip-egg-info
if and when we will build in place, use a temporary directory instead.Question to reviewers: can one think if there was a good reason to generate that
pip-egg-info
inside source dir?If this PR sounds reasonable, one can further think about always generating legacy metadata in a temporary directory, even in the editable case. Indeed it looks like[update] this simplification in now part of this PRsetup.py develop
always runs egg_info even if the directory is already present. This would vastly simplify this area of the code.