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

fix PEP 518 support when pip is installed in the user site #5227

Merged
merged 1 commit into from
Apr 18, 2018

Conversation

benoit-pierre
Copy link
Member

Do not use the build environment isolation during the build dependencies installation.

Note: included are the changes originally proposed in #5189 as well as an additional patch fixing the virtualenvs used by the testsuite so user site packages are correctly handled.

Fix #5224.

@benoit-pierre benoit-pierre changed the title fix PEP 518 when pip is installed in the user site fix PEP 518 support when pip is installed in the user site Apr 14, 2018
@pv
Copy link

pv commented Apr 14, 2018

I tested this indeed fixes the failure I reported #5224.

@pv
Copy link

pv commented Apr 14, 2018

Looking at this, I was at first worried the build isolation would be important to have for the dependencies, but IIUC each build dep anyway gets its own isolated environment if it has a pyproject.toml, so the logic is OK as far as I see.

@pradyunsg
Copy link
Member

It'd be nicer if you reopen the earlier PR and separate these changes from those. I was about to go to merge those but saw you'd closed that PR. :/

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

I feel it'd be cleaner to just move "installation of build requirements" to become a method of BuildEnvironment (and update NoOpBuildEnvironement) accordingly.

@benoit-pierre
Copy link
Member Author

I'd rather do those cleanups in a dedicated PR. I planned to do one to remove the BuildEnvironment.no_clean code too, it's not used.

I can move the first 2 commits in a dedicated PR.

@pradyunsg
Copy link
Member

pradyunsg commented Apr 15, 2018 via email

@benoit-pierre
Copy link
Member Author

OK, so #5235 should be merged first.

@benoit-pierre benoit-pierre mentioned this pull request Apr 16, 2018
@benoit-pierre benoit-pierre force-pushed the fix_pep518,_again branch 2 times, most recently from a816c4b to 02b678f Compare April 16, 2018 20:57
Do not use the build environment isolation during the build dependencies installation.
@pfmoore
Copy link
Member

pfmoore commented Apr 17, 2018

@pradyunsg:

I feel it'd be cleaner to just move "installation of build requirements" to become a method of BuildEnvironment (and update NoOpBuildEnvironement) accordingly.

Do you still wish this to be done? Could this be deferred to a tidy-up PR once 10.0.1 is released? I'm reluctant to delay 10.0.1 over a cosmetic issue in the code.

@pradyunsg pradyunsg dismissed their stale review April 17, 2018 17:27

Happy to let this go through as is for now.

@pfmoore pfmoore merged commit d8172b9 into pypa:master Apr 18, 2018
@benoit-pierre benoit-pierre deleted the fix_pep518,_again branch September 23, 2018 17:45
@lock
Copy link

lock bot commented Jun 1, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot added the auto-locked Outdated issues that have been locked by automation label Jun 1, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build isolation fails with system-wide pip<10 and user-site pip==10.0.0
4 participants