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

Make editable requirements use relative paths where appropriate. #507

Closed
wants to merge 4 commits into from
Closed

Make editable requirements use relative paths where appropriate. #507

wants to merge 4 commits into from

Conversation

orf
Copy link

@orf orf commented May 12, 2017

#204

When pip-compile is given a relative editable dependency, like -e ./some_module/, it is resolved to the full absolute path by pip under the hood. This is not what we want or need in most cases.

As the path resolution happens inside pip and it might be a bit complicated to add a change in that project, this merge request works around the issue by modifying the format_requirement to check that if the editable dependency uses the file: scheme and if the location of the editable dependency exists within a subdirectory of the current working directory then we format the dependency using the relative path from the current working directory.

I think this is the best place to add this change and requires less potential breakage and internal changes. I'm not too sure on if we should use the current directory or the path from the output file directory, but that's a small detail.

I also did a small refactoring of the tests by adding fixtures to return the paths of the packages within the tests/fixtures/ directory rather than duplicating various os.path.join calls everywhere.

Contributor checklist
  • Provided the tests for the changes
  • Added the changes to CHANGELOG.md
  • Requested (or received) a review from another contributor

@davidovich davidovich self-requested a review May 14, 2017 02:25
Copy link
Contributor

@davidovich davidovich left a comment

Choose a reason for hiding this comment

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

Thanks for this

Copy link
Member

@ulope ulope left a comment

Choose a reason for hiding this comment

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

Unfortunately this only works for the "happy case" where the current working directory happens to be the common root of the requirements file and the local packages.

The local packages path would need to be resolved against the path of the requirements file from which it was referenced. However from briefly looking at the code it seems to me that the path isn't available after parsing has finished. So I'm not quite sure how to solve this problem :/

@orf
Copy link
Author

orf commented May 16, 2017

It's also common to have a requirements file in a directory like project/requirements/{debug,prod}.in, containing only the requirements. In this case the editable requirements are not in a sub-directory of their referencing requirements and so the current implementation wouldn't work.

I guess to cover this case we could just say 'it resolves to a relative path if it is in a sub-directory of the requirements file' and be done with it (providing we can fix the issue you described). People who need this fix can then move the files as needed to make it work?

path = ireq.link.path
if ireq.link.scheme == 'file' and is_subdirectory(os.getcwd(), path):
# If the ireq.link is relative to the current directory then output a relative path
path = 'file:' + os.path.join('.', os.path.relpath(path))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that there are Windows idiosyncrasies that would be taken care of if you used pathname2url (like in the original pip code you pointed at:

urllib_parse.urljoin('file:', urllib_request.pathname2url(path)) 

this should also help the failing Windows build.


@fixture
def minimal_wheels_dir():
return os.path.join(os.path.split(__file__)[0], 'fixtures', 'minimal_wheels')
Copy link
Contributor

Choose a reason for hiding this comment

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

perfect

def format_requirement(ireq, marker=None):
"""
Generic formatter for pretty printing InstallRequirements to the terminal
in a less verbose way than using its `__str__` method.
"""
if ireq.editable:
line = '-e {}'.format(ireq.link)
path = ireq.link.path
if ireq.link.scheme == 'file' and is_subdirectory(os.getcwd(), path):
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not certain that cwd is a correct base here. I could call pip-compile outside of the directory which contains the requirement file, and that would leave us with a non installable compiled requirements.txt. Am I mistaken in thinking that the base should always be relative to the directory of the input file ?

suutari-ai pushed a commit to suutari/prequ that referenced this pull request Oct 8, 2017
This branch implements support for using relative paths as requirement
entries (in setup.cfg or in requirements.in).

The base feature comes from a pip-tools PR (jazzband#507) by
Tom Forbes.  Thanks Tom!

Prequ supports also non-editable URLs and therefore the feature needed
some polishing.  The changes in this branch should make the feature work
nicely as a whole.
@atugushev atugushev added needs rebase Need to rebase or merge and removed needs review labels Sep 26, 2019
@orf orf closed this Dec 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs rebase Need to rebase or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants