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

Use a realpath for the temporary build directory. #3701

Merged
merged 1 commit into from May 20, 2016

Conversation

Projects
None yet
3 participants
@zvezdan
Contributor

zvezdan commented May 19, 2016

Some systems have /tmp symlinked which confuses custom builds, such as
numpy. This ensures that real path is passed and that such builds
resolve their paths correctly during build and install.

Added test for the change and also for the previous related fix: #707


This was migrated from pypa/pip#3079 to reparent it to the master
branch. Please see original pull request for any previous discussion.

Use a realpath for the temporary build directory.
Some systems have /tmp symlinked which confuses custom builds, such as
numpy. This ensures that real path is passed and that such builds
resolve their paths correctly during build and install.

Added test for the change and also for the previous related fix: #707

---

*This was migrated from pypa/pip#3079 to reparent it to the ``master``
branch. Please see original pull request for any previous discussion.*

@dstufft dstufft merged commit 4e2c1fa into pypa:master May 20, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 21, 2016

Member

This seems to have caused the pypy tests on master to fail. Most seem related to a temporary PyPI issue, but there's a Unicode error in there that seems to still be happening.

Member

pfmoore commented May 21, 2016

This seems to have caused the pypy tests on master to fail. Most seem related to a temporary PyPI issue, but there's a Unicode error in there that seems to still be happening.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 21, 2016

Member

Hmm, that's strange. The tests were passing on PyPy before this landed.

Member

dstufft commented May 21, 2016

Hmm, that's strange. The tests were passing on PyPy before this landed.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 21, 2016

Member

Yeah, may not be this change. I just had a failure from my latest PR, and when I checked, the builds on master had been failing since this one was committed - but that may be a coincidence.

Member

pfmoore commented May 21, 2016

Yeah, may not be this change. I just had a failure from my latest PR, and when I checked, the builds on master had been failing since this one was committed - but that may be a coincidence.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 21, 2016

Member

I wonder - could a server used in the test be returning non-ASCII data? Can't see the error at the moment because I see you restarted the test.

Member

pfmoore commented May 21, 2016

I wonder - could a server used in the test be returning non-ASCII data? Can't see the error at the moment because I see you restarted the test.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft May 21, 2016

Member

I didn't restart the test, I did merge something else though :)

It's totally possible something is returning non ASCII data.

Member

dstufft commented May 21, 2016

I didn't restart the test, I did merge something else though :)

It's totally possible something is returning non ASCII data.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 21, 2016

Member

Ah, OK. I'll look at the test again when it completes, and see if I can work out what's going on.

Member

pfmoore commented May 21, 2016

Ah, OK. I'll look at the test again when it completes, and see if I can work out what's going on.

@zvezdan

This comment has been minimized.

Show comment
Hide comment
@zvezdan

zvezdan May 21, 2016

Contributor

@pfmoore
This patch was made in September last year (see the original patch number above)
It had been rebased numerous times and never had issues with passing tests after each rebase.
It passed all the tests before the merge too.

Also, if you look at the code change, the patch is dealing with directories and only strings used in the newly added tests are the prefix/suffix for the temporary directory names. They are plain strings and will be treated correctly by Python 2 or 3.

Thank you for checking what else could cause the failures you're seeing.

Contributor

zvezdan commented May 21, 2016

@pfmoore
This patch was made in September last year (see the original patch number above)
It had been rebased numerous times and never had issues with passing tests after each rebase.
It passed all the tests before the merge too.

Also, if you look at the code change, the patch is dealing with directories and only strings used in the newly added tests are the prefix/suffix for the temporary directory names. They are plain strings and will be treated correctly by Python 2 or 3.

Thank you for checking what else could cause the failures you're seeing.

@pfmoore

This comment has been minimized.

Show comment
Hide comment
@pfmoore

pfmoore May 21, 2016

Member

@zvezdan It appears to have cleared itself up now (or @dstufft fixed it - I didn't get to it before it started working, anyway). Sorry if I gave the impression I thought your PR was faulty - I didn't mean to, I was more wondering if anyone else had noticed the failure after this went in and was looking into it.

Anyway, it's gone away now, so no problem :-)

Member

pfmoore commented May 21, 2016

@zvezdan It appears to have cleared itself up now (or @dstufft fixed it - I didn't get to it before it started working, anyway). Sorry if I gave the impression I thought your PR was faulty - I didn't mean to, I was more wondering if anyone else had noticed the failure after this went in and was looking into it.

Anyway, it's gone away now, so no problem :-)

@zvezdan

This comment has been minimized.

Show comment
Hide comment
@zvezdan

zvezdan May 21, 2016

Contributor

@pfmoore
Thanks for checking. No need to apologize. Anyone's PR can be faulty (including mine) and it's important to check the changes that could potentially cause issues. This forced us all to look at those Travis errors and find the root cause -- that's always a good thing. :-)

Thanks again!

Contributor

zvezdan commented May 21, 2016

@pfmoore
Thanks for checking. No need to apologize. Anyone's PR can be faulty (including mine) and it's important to check the changes that could potentially cause issues. This forced us all to look at those Travis errors and find the root cause -- that's always a good thing. :-)

Thanks again!

waisbrot pushed a commit to waisbrot/pip that referenced this pull request Jul 5, 2016

Use a realpath for the temporary build directory. (#3701)
Some systems have /tmp symlinked which confuses custom builds, such as
numpy. This ensures that real path is passed and that such builds
resolve their paths correctly during build and install.

Added test for the change and also for the previous related fix: #707

---

*This was migrated from pypa/pip#3079 to reparent it to the ``master``
branch. Please see original pull request for any previous discussion.*

zvezdan added a commit to zvezdan/pygradle that referenced this pull request Jun 16, 2017

Upgrade setup and build requirements for Python 3.6.
With previously committed changes we now have everything in place for
upgrade of setuptools, virtualenv, pip, and pex. We also upgrade pytest
and flake8 to match versions used internally at LinkedIn.

Also, bumped the version to 0.6.0 and removed the issue with pip-7
because we now use pip-9 with our patch applied
(pypa/pip#3701).

qlan2 pushed a commit to qlan2/pivy-importer that referenced this pull request Mar 19, 2018

Upgrade setup and build requirements for Python 3.6.
With previously committed changes we now have everything in place for
upgrade of setuptools, virtualenv, pip, and pex. We also upgrade pytest
and flake8 to match versions used internally at LinkedIn.

Also, bumped the version to 0.6.0 and removed the issue with pip-7
because we now use pip-9 with our patch applied
(pypa/pip#3701).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment