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.

Sign up for GitHub

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

Jump to bottom

Fix invalid header path when --root is set #8881

Closed
wants to merge 1 commit into from
Closed

Fix invalid header path when --root is set #8881

wants to merge 1 commit into from

Conversation

lkollar
Copy link
Contributor

@lkollar lkollar commented Sep 15, 2020

When we invoke setuptools with a --root, the header directory we take
from the Scheme object already contains the root as a prefix. Passing
both --root and --install-headers will result in setuptools adding
the root prefix twice in the header path.

As the Scheme object already has all directories relative to --root,
we can pass these separately with the --install-* setuptools
arguments and drop the --root argument.

Closes: #8477

@lkollar lkollar force-pushed the fix-install-headers branch from 4479542 to 7959ef5 Compare September 15, 2020 20:20
@pradyunsg pradyunsg added the project: setuptools Related to setuptools label Sep 16, 2020
@lkollar lkollar force-pushed the fix-install-headers branch from 7959ef5 to 5f2df9f Compare September 17, 2020 21:03
@lkollar lkollar marked this pull request as ready for review September 17, 2020 21:05
@lkollar lkollar force-pushed the fix-install-headers branch from 5f2df9f to 9ed55ef Compare September 17, 2020 21:08
@lkollar lkollar force-pushed the fix-install-headers branch 6 times, most recently from 0338073 to 9b0f5df Compare September 26, 2020 08:58
@lkollar
Copy link
Contributor Author

lkollar commented Sep 28, 2020

This is ready for review. I simplified the test case to only check whether the installed files are all under the same virtual environment base installation as otherwise the test would have to account for the different locations used across platforms and interpreter implementations.

pradyunsg
pradyunsg reviewed Sep 28, 2020
View reviewed changes
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.

Seems OK to me, however, this is touching the legacy code and I'm not 100% sure about the de-facto setuptools <-> pip contract around this area.

(anything marked with nit: is a nitpick and doesn't need discussion/addressing unless you agree with it and making that change isn't too much work)

src/pip/_internal/operations/install/legacy.py Outdated Show resolved Hide resolved
@pradyunsg pradyunsg requested a review from chrahunt September 28, 2020 09:14
@lkollar lkollar force-pushed the fix-install-headers branch 2 times, most recently from cf00cbb to 51c0eec Compare October 7, 2020 09:53
@lkollar
Copy link
Contributor Author

lkollar commented Oct 7, 2020

Thanks @pradyunsg, I've updated the PR.

@lkollar
Copy link
Contributor Author

lkollar commented Oct 27, 2020

@pradyunsg Do you know of anyone else who could better review the change? It would be amazing if this could be part of the next release. This issue breaks all users who rely on the --root argument.

@pradyunsg
Copy link
Member

Could you rebase this PR on the current master branch? Chris is definitely the best person I can think of.

@lkollar
Fix invalid header path when --root is set
ae31ac8 
When we invoke setuptools with a `--root`, the header directory we take
from the Scheme object already contains the root as a prefix. Passing
both `--root` and `--install-headers` will result in setuptools adding
the root prefix twice in the header path.

As the Scheme object already has all directories relative to `--root`,
we can pass these separately with the `--install-*` setuptools
arguments and drop the `--root` argument.

As the specific locations differ across interpreters and platforms, the
included test only checks that scripts, headers and library contents are
installed under the specified package root. This is not the case without
the fix, as some files will be installed under the duplicated root path,
outside the test `venv`.
@lkollar lkollar force-pushed the fix-install-headers branch from 51c0eec to ae31ac8 Compare October 28, 2020 15:18
@lkollar lkollar mentioned this pull request Jan 19, 2021
Open
@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 19, 2021
@lkollar
Copy link
Contributor Author

lkollar commented Feb 23, 2021

This PR is not relevant anymore in light of #8477 (comment).

@lkollar lkollar closed this Feb 23, 2021
@lkollar lkollar deleted the fix-install-headers branch February 23, 2021 10:16
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@pradyunsg pradyunsg pradyunsg left review comments

@uranusjr uranusjr uranusjr left review comments

@chrahunt chrahunt Awaiting requested review from chrahunt

Assignees
No one assigned
Labels
needs rebase or merge PR has conflicts with current master project: setuptools Related to setuptools
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

pip installs headers to incorrect location when using '--root'
4 participants
@lkollar @pradyunsg @BrownTruck @uranusjr