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

pip wheel . does not follow symlinks whereas python setup.py bdist_wheel does #3500

Closed
sbidoul opened this issue Feb 21, 2016 · 29 comments
Closed
Labels
C: wheel The wheel format and 'pip wheel' command type: bug A confirmed bug or unintended behavior type: docs Documentation related

Comments

@sbidoul
Copy link
Member

sbidoul commented Feb 21, 2016

When the package source code contains symlinks, python setup.py bdist_wheel follows them and store their content in the resulting wheel.

pip wheel . ignores them.

@sbidoul sbidoul changed the title pip wheel does not follow symlinks whereas python setup.py bdist_wheel does pip wheel . does not follow symlinks whereas python setup.py bdist_wheel does Feb 21, 2016
@sbidoul
Copy link
Member Author

sbidoul commented Feb 22, 2016

I was apparently using a wrong branch. Sorry for the noise.

@sbidoul sbidoul closed this as completed Feb 22, 2016
@sbidoul sbidoul reopened this Feb 23, 2016
@sbidoul
Copy link
Member Author

sbidoul commented Feb 23, 2016

After all I could reproduce. The directory structure must be as follow

  • linked
  • setup
  • setup/setup.py
  • setup/package -> ../linked

In the setup directory, when runnning setup.py bdist_wheel, the package is properly included, while running pip wheel . does not include it.

The root cause seems to be that bdist_wheel builds in place, while pip wheel copies the directory to another temporary place where it builds, and it does that without following the symlinks.

So my first question is why pip wheel does this copy when the source is a local directory?

@dstufft
Copy link
Member

dstufft commented Mar 30, 2017

This is going to effectively be as intended. We copy the directory so as not to modify the source directory and to contain all of our modifications to a throw away temporary directory.

@dstufft dstufft closed this as completed Mar 30, 2017
@sbidoul
Copy link
Member Author

sbidoul commented Mar 30, 2017

@dstufft would it make sense to follow symlinks when copying?

@dstufft
Copy link
Member

dstufft commented Mar 30, 2017

@sbidoul Without thinking too hard about it I could see allowing that, but I'd need to make sure we weren't doing that for a reason.

@RonnyPfannschmidt
Copy link
Contributor

@sbidoul that would present as massive security issue in many cases (symlink attacks are a pain)

@dstufft
Copy link
Member

dstufft commented Mar 30, 2017

@RonnyPfannschmidt I'm not sure that's true, since we're going to be executing a setup.py anyways after we copy it, I doubt there's anything you can do with symlinks you couldn't just do in the Python file itself.

@RonnyPfannschmidt
Copy link
Contributor

@dstufft a paranoid possibility is using a symlink attack to turn a package that ships web served data into a package that ships web served data and some private details of the package builder, but building wheels in isolation should elevate that

@xavfernandez xavfernandez added the C: wheel The wheel format and 'pip wheel' command label Mar 30, 2017
@sbidoul
Copy link
Member Author

sbidoul commented Mar 30, 2017

@RonnyPfannschmidt @dstufft my point is that setup.py bdist_wheel does follow symlinks whereas pip wheel . does not. In my understanding the resulting wheel must be identical. So if there is any security implication (which I doubt) it is already present in setuptools.

@sbidoul
Copy link
Member Author

sbidoul commented Mar 30, 2017

@dstufft @xavfernandez does my last comment warrant reopening this?

@r-barnes
Copy link

r-barnes commented Jan 1, 2018

I've just been burned by this symlink issue.

At the very least, wheel should provide a warning that it hasn't followed symlinks so the user knows what's wrong.

@pradyunsg
Copy link
Member

I think we can add a small documentation note to pip wheel as a short term workaround.

@pypa/pip-committers What do we want to do here? Following symlinks while copying seems fine to me, if that's what setuptools is doing.

@pradyunsg pradyunsg self-assigned this Mar 4, 2018
@pradyunsg pradyunsg added type: bug A confirmed bug or unintended behavior type: docs Documentation related state: needs discussion This needs some more discussion S: awaiting response Waiting for a response/more information labels Mar 4, 2018
@RonnyPfannschmidt
Copy link
Contributor

setuptools doesnt make a copy of the source folder to begin with

@realjo
Copy link

realjo commented Apr 4, 2018

sbidoul commented on 30 Mar 2017:
... would it make sense to follow symlinks when copying?

It would be sufficient to create a symlink in the copy pointing to the absolute path of the target of the symlink in the source which would reduce the attack risk IMO.

@pradyunsg pradyunsg removed C: wheel The wheel format and 'pip wheel' command S: awaiting response Waiting for a response/more information state: needs discussion This needs some more discussion type: docs Documentation related labels May 11, 2018
@sbidoul
Copy link
Member Author

sbidoul commented Aug 25, 2019

I now think this issue or variants of it arise in any situation where

  • setup.py / pyproject.toml is not at the root of the project
  • the build depends on resources outside of the setup.py / pyproject.toml subtree

I found three variants:

  1. build needs resources that are symlinks to files/directories outside of the subtree (the case which prompted this issue)
  2. build needs the git repository (eg when using setuptools_scm), and .git/ is in a parent directory not copied to the temp dir by pip
  3. build relies on the subdirectory name (somewhat exotic maybe, yet I have such as case where I want to create a custom build backend and part of the metadata depends on the subdirectory name)

Since pip copies the subtree to a temporary location before building, when running pip install . or pip wheel . resources that reside out of the subtree are not available for building.

I think this happens both for pep517 and legacy builds.

Note that when running pip install git+http://g.c/repo/project#egg=project&subdirectory=subdir it works fine because the whole project is available (I think no additional copy takes place in that case).

So while it could be possible to fix this particular issue (case 1.) with some strategy to preserve symlinks when copying to the temporary directory, this would not solve 2. nor 3.

I'm not sure if pep517 build isolation mandates working on a temporary copy of the source tree.

Maybe a possible approach could be to have a way for build backends to declare that they are safe for in-place builds (in an isolated build environment, but in the same source tree). Build backend would then have a way to promise to front ends they will not modify the source tree. And pip could use that information to decide to build in place without copying to a temporary directory.

Do you think it's a track worth investigating further?

@pfmoore
Copy link
Member

pfmoore commented Aug 25, 2019

To be honest, I think the best way of fixing this issue is to simply switch to building a sdist in place, and then extracting that sdist to the temporary directory. We've been talking about doing that for ages now, and I think we should bite the bullet and do it. The best approach is probably to do it for PEP 517 build only, so that issues like this won't get fixed for legacy builds, but conversely we will limit any potential issues to people using PEP 517, who are in a transition period anyway.

I've argued for build via sdist in the past, so I'm willing to work on it - with the proviso that my time is limited currently, so I can't promise it will go quickly...

PS The question of in-place builds was extensively debated during the PEP 517 discussions. I don't recall what conclusions we came to, but I do know we didn't come to a consensus (which is why PEP 517 doesn't address the issue). I'd strongly recommend researching that history before opening that question again (and I'd also caution that I'm personally not sure that this issue is significant enough to justify doing so... ☹️)

@sbidoul
Copy link
Member Author

sbidoul commented Aug 25, 2019

@pfmoore Thanks, I'll do some research to find those discussions about in place builds.

A quick question comes to mind, though: if builds go via sdist, does it mean building the sdist in place is considered "safer" than building the wheel in place? As an example, the setuptools build backend does modify the source tree by creating at least the .egg-info directory.

Also since this copy to a temp directory may influence the build outcome, it might be useful to clarify what frontends can and cannot do in that respect. For instance in this specific example, I just tested, and I have a situation where pip wheel . (in pep 517 mode) fails while python -m pep517.build -b . succeeds (because the latter builds in place). We may want to weed out such ambiguities while the spec is young enough?

I can't really judge the significance of the issue. My intuition is that it is relevant for all monorepo projects where, for instance, the python binding of the project is only one part of the system that lives in a a subdirectory.

@pfmoore
Copy link
Member

pfmoore commented Aug 25, 2019

if builds go via sdist, does it mean building the sdist in place is considered "safer" than building the wheel in place?

That's honestly a very good question, and not one anyone's really thought about, I suspect!

We may want to weed out such ambiguities while the spec is young enough?

Definitely, yes. The reason it isn't in PEP 517 yet is largely that everyone was burned out on the discussion, and we needed to get something out that defined the key points, even if it left certain things open to interpretation. Maybe this is the point at which we need another round of discussion on this.

Regarding significance, I agree - all I'm really going off is "it hasn't generated a lot of bug reports over the years". But that's not necessarily a good indicator, it's merely all we've got :-)

@sbidoul
Copy link
Member Author

sbidoul commented Aug 26, 2019

Maybe this is the point at which we need another round of discussion on this.

I can volunteer to open a thread about that on Discourse and try to summarize the matter.
I'll wait a little bit for my other PyPA topics (PEP and PRs) to land though, as I fear to have too many of them in flight at the same time :)

@pradyunsg pradyunsg assigned pradyunsg and unassigned pradyunsg Aug 27, 2019
@chrahunt
Copy link
Member

chrahunt commented Sep 2, 2019

Some discussion about building via sdist is in #2195.

@sbidoul
Copy link
Member Author

sbidoul commented Nov 3, 2019

More discussion about building via sdist on discuss.python.org.

@sbidoul
Copy link
Member Author

sbidoul commented Nov 3, 2019

I wonder if an option to tell pip to build in place would make sense.

@pradyunsg
Copy link
Member

It would, yes.

@sbidoul
Copy link
Member Author

sbidoul commented Apr 13, 2020

Closed via #7882 (build in place).

@sbidoul sbidoul closed this as completed Apr 13, 2020
@pradyunsg
Copy link
Member

We have now (per #7951) published a beta release of pip, pip 20.1b1. This release includes #7882, which implemented a solution for this issue.

I hope participants in this issue will help us by testing the beta and checking for new bugs. We'd like to identify and iron out any potential issues before the main 20.1 release on Tuesday.

I also welcome positive feedback along the lines of "yay, it works better now!" as well, since the issue tracker is usually full of "issues". :)

@pradyunsg
Copy link
Member

Unfortunately, there have been a number of issues with the implementation of in-place builds (which are being tracked under #7555) which means that for now, we need to revert #7882. As a result, this issue will become a problem again, and we'll therefore be reopening it. Longer-term, we hope to have a solution that addresses the issues that in-place builds solved, but without the impact on other workflows that the current solution had.

Sorry for the disruption that this will cause.

@pradyunsg pradyunsg reopened this May 14, 2020
MarDiehl added a commit to damask-multiphysics/DAMASK that referenced this issue Jun 23, 2021
pypa/pip#3500
Not the nicest solution, but works and keeps single source of truth
@sbidoul
Copy link
Member Author

sbidoul commented Jun 5, 2022

This is resolved now that pip builds in tree.

@sbidoul sbidoul closed this as completed Jun 5, 2022
@pradyunsg pradyunsg removed the state: awaiting PR Feature discussed, PR is needed label Jun 5, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: wheel The wheel format and 'pip wheel' command type: bug A confirmed bug or unintended behavior type: docs Documentation related
Projects
None yet
Development

No branches or pull requests

9 participants