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

Write relative find-links opts to output file #453

Closed
wants to merge 1 commit into from

Conversation

suutari-ai
Copy link
Contributor

@suutari-ai suutari-ai commented Feb 11, 2017

If input file or command line has --find-links options that point to directories which are relative to the destination file (i.e. relative path from destination directory to find-links directory does not contain any ../ components), then those should be written to the output file.

Now --find-links options in the source file work similarly as e.g. the --extra-index-url options. This makes it possible to make deploy scripts to install from requirements.txt without knowing about the find-links options.

If input file or command line has `--find-links` options that point to
directories which are relative to the destination file (i.e. relative
path from destination directory to find-links directory does not contain
any `../` components), then those should be written to the output file.

Now `--find-links` options in the source file work similarly as e.g.
the `--extra-index-url` options.  This makes it possible to make deploy
scripts to install from `requirements.txt` without knowing about the
find-links options.
@suutari
Copy link
Contributor

suutari commented Jan 7, 2018

This is also rebased onto master now. Any reviewers?

This feature will be useful with the feature implemented in PR #464.

dst_dir = os.path.dirname(self.dst_file)
for abs_link in self.find_links:
rel_link = os.path.relpath(abs_link, start=dst_dir)
if not rel_link.startswith(os.path.pardir + os.path.sep):
Copy link
Member

Choose a reason for hiding this comment

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

Let's assume that user has /app/stuff/requirements.txt and /app/links/. In that case relative link would be ../links and this link will be omitted. Do you think it's okay?

@atugushev atugushev added enhancement Improvements to functionality needs rebase Need to rebase or merge labels Sep 26, 2019
@AndydeCleyre
Copy link
Contributor

This idea of using relative paths for find-link paths iff the destination txt's parent folder is an ancestor of the link path is interesting. That's what's intended in this PR, right?

I'd like to see a little more discussion just to confirm that's the best behavior for users, rather than being dependent on the user-provided forms of the paths in some way. I don't often use the option myself so can't offer much insight, but I'd imagine there are cases where a relative path is desired but the above condition doesn't hold.

I don't see a relevant issue open (or closed) about this, but please link it if I missed one. I think if we can get #1329 reviewed and settled, we should then try to establish a consensus on proper path behavior for find-links.

@RuRo
Copy link

RuRo commented Apr 5, 2021

I came here from the "pip-compile absolute links when given relative path" issue. In my opinion, find-lnks, editable and relative vs absolute paths are quite advanced and very niche use-cases.

I expect that different people will want different behaviours for relative paths and such. It is unlikely in my opinion, that you can provide a single sane default behaviour, that satisfies everyone.

Currently, (at least for editable) you can "choose" these different behaviours by using . vs file:.. In my opinion, this behaviour is brittle and not very intuitive.

I would propose introducing some way to explicitly specify the desired behaviour. Something along the lines of

some-package
-e . # pip-tools: absolute
-e ./mydep # pip-tools: relative

in requirements.in. And the same syntax for find-links (maybe also with a pip-tools: inline idk). This would allow the users to specify the desired behaviour.

More importantly, this would also introduce a generic way to specify behaviour in ambiguous cases.

TL;DR: Instead of trying to come up with some convoluted "clever" logic for automagically choosing the "correct" behaviour, consider providing 1 fixed default behaviour and allowing the user to explicitly specify alternatives via noqa: style comments.

@AndydeCleyre
Copy link
Contributor

AndydeCleyre commented Apr 5, 2021

@RuRo

Thanks!

Can you bring some of that feedback (for non-find-links paths) to #1329, which is currently using a behavior different from what you describe, but does allow users to specify behavior via the form of path they provide (rather than a comment)? Please have a look there, and leave a note if you think a comment parsing system would be better than what's up for review.

@ssbarnea
Copy link
Member

ssbarnea commented Jun 5, 2021

Closing as if this was not fixed in 4 years, I doubt it will ever be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvements to functionality needs rebase Need to rebase or merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants