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

Build meta: fixes and cleanups #1175

Merged
merged 10 commits into from Nov 14, 2017
Merged

Build meta: fixes and cleanups #1175

merged 10 commits into from Nov 14, 2017

Conversation

@ghost
Copy link

@ghost ghost commented Oct 15, 2017

Attached are some minor cleanups and fixes to the build_meta module that I added in #1143.

xoviat added 5 commits Oct 15, 2017
In some cases (specifically when pip imports this module in a virtualenv),
pkg_resources can already be imported, causing setuptools to load
entry_points from an older version. Here, we re-initialize the master
working set to fix the case where the entry points from an older
setuptools are loaded.
This change is a small simplification that simply creates the
egg_info directory in the egg_base location; it's a minor
cleanup that results in some read and it helps with
read-only directories (the egg_info directory is uncontrollable).
@ghost
Copy link
Author

@ghost ghost commented Oct 15, 2017

Okay, this should be ready to go.

@ghost ghost mentioned this pull request Oct 16, 2017
@@ -64,11 +66,13 @@ def patch(cls):
def _run_setup(setup_script='setup.py'):
# Note that we can reuse our build directory between calls
# Correctness comes first, then optimization later
_initialize_master_working_set()

This comment has been minimized.

@benoit-pierre

benoit-pierre Oct 16, 2017
Member

This is not necessary anymore, no?

@jaraco
Copy link
Member

@jaraco jaraco commented Oct 16, 2017

This PR needs more detail. In particular, it needs a bug report or comment describing the undesirable behavior this is correcting. I see some description in the commit message of the first commit (ffb2e69)... and I'd like to know more. In particular, what are the two setuptools versions under which different entry points are loaded? I feel like I've encountered some of these issues before and want to be sure we're coordinating approaches.

@benoit-pierre
Copy link
Member

@benoit-pierre benoit-pierre commented Oct 16, 2017

Related: #1174.

Adding 1 or 2 tests for the changes to _run_setup would be great, check if those examples can be run:

variable = True
def function():
  return variable
assert __name__ == '__main__'
@ghost
Copy link
Author

@ghost ghost commented Oct 16, 2017

@benoit-pierre Can you explain the locals() and __main__ thing here?

@ghost
Copy link
Author

@ghost ghost commented Oct 16, 2017

In particular, it needs a bug report or comment describing the undesirable behavior this is correcting.

In particular, some setup scripts (particularly here) failed because the __name__ attribute wasn't set to __main__. In addition, passing locals() executes the script in the context of the function, which doesn't pass in the classes defined in the module as part of the setup.py namespace.

The other change with respect to dist_info is a minor cleanup that reduces the number of times that files are copied and, in the case of a read-only directory with a setup.py, doesn't fail trying to create .egg-info.

The pkg_resources change has been dropped because it actually isn't necessary and I am afraid it would bog down this PR.

@ghost
Copy link
Author

@ghost ghost commented Oct 16, 2017

Tests have been added.

@jaraco Hopefully I've done the right thing.

@ghost
Copy link
Author

@ghost ghost commented Oct 16, 2017

Also many thanks to @benoit-pierre for the excellent PR reviews.

@ghost
Copy link
Author

@ghost ghost commented Oct 19, 2017

@benoit-pierre I have found another issue with the hook. To be perfectly clear, this pull request should still be merged even if it is not fixed because it fixes some of the bugs, but not all of them. Specifically, it appears that in some cases, (see here), .egg-info is not placed in the egg-base directory, but rather a sub-directory (specifically 'src').

What I'm going to to specifically here to resolve this problem is if there is no dist-info in the current directory, then I'll look in a subdirectory for the dist-info. However, long term, I think egg_info should be patched to specifically recover the directory that it wrote. That type of fix is risker at this point, and I really do not want to further delay the PR.

This code is a bit ugly, but it's also been tested with the pip test suite
It's not the best solution long term (the best solution is to get the egg_info
directory directly from egg_info), but it works for now and avoids
technical risk.
@ghost
Copy link
Author

@ghost ghost commented Nov 10, 2017

@jaraco ping.

@jaraco jaraco merged commit 823780e into pypa:master Nov 14, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ghost ghost deleted the build_meta branch Nov 14, 2017
jaraco added a commit that referenced this pull request Nov 14, 2017
@jaraco
Copy link
Member

@jaraco jaraco commented Nov 14, 2017

Released as v36.7.3

@ghost
Copy link
Author

@ghost ghost commented Nov 14, 2017

Thanks. It looks like the pip side has gone off the rails though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants