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

Skip constraint path check #2038

Merged
merged 1 commit into from
Mar 4, 2024
Merged

Skip constraint path check #2038

merged 1 commit into from
Mar 4, 2024

Conversation

honnix
Copy link
Contributor

@honnix honnix commented Dec 29, 2023

pip can read constraint file via HTTP so pip-compile doesn't need to restrict that to a local file. Removing the option type makes it a plain string so users can pass in an URL. This of course skip local file validation, but the compilation would fail anyway if the file is not readable, no matter local or remote.

Closes #2040

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@chrysle
Copy link
Contributor

chrysle commented Jan 2, 2024

I don't think removing the type altogether is the right solution. Failing early with a definite traceback is better, in my opinion.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

I don't think removing the type altogether is the right solution. Failing early with a definite traceback is better, in my opinion.

It's not that early I guess but I can understand this.

@honnix honnix mentioned this pull request Jan 2, 2024
4 tasks
@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

I tried early validation in #2040 but I honestly feel it is overshooting.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

If I understand it correctly, this command line option is more or less transparently passed down to pip internal, so it doesn't help much applying an additional layer of validation.

@chrysle
Copy link
Contributor

chrysle commented Jan 2, 2024

Now I wasn't urging you to write an own enhanced parameter type! You must need this feature badly ;-).

I think I'm going to ask over at click whether they want to extend click.Path for URLs permanently. Until then, I'd like to hear other opinions.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

@chrysle Haha, I never tried extending click type, so it was a fun exercise.

Until then, I'd like to hear other opinions.

Yeah that makes sense.

@webknjaz
Copy link
Member

webknjaz commented Jan 2, 2024

Honestly, this strikes me as dangerous and it removes the immutability/reproducibility property of the constraint files. This makes it possible to influence the resolved deptree by modifying an externally managed file on the internet which subsequently opens up a can of worms, allowing for rollback attacks.

If this is implemented, it shouldn't behave like this by default and should only be toggled by the end-user with a sufficiently cautious flag name like --allow-dangerous-insecure-and-unverified-constraints-for-rollback-attacks.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

Honestly, this strikes me as dangerous and it removes the immutability/reproducibility property of the constraint files. This makes it possible to influence the resolved deptree by modifying an externally managed file on the internet which subsequently opens up a can of worms, allowing for rollback attacks.

If this is implemented, it shouldn't behave like this by default and should only be toggled by the end-user with a sufficiently cautious flag name like --allow-dangerous-insecure-and-unverified-constraints-for-rollback-attacks.

It is a valid concern, but this is not a feature of pip-compile but pip itself. pip always supports reading a file from HTTP(S) for constraints and requirements. This feature has been used by for example Airflow for some time: https://raw.githubusercontent.com/apache/airflow/constraints-2.4.3/constraints-3.10.txt. So there is no reason that pip-compile should block it. Today users of pip-compile can already use a constraints file from HTTP(S) in a *.in file header for example.

@webknjaz
Copy link
Member

webknjaz commented Jan 2, 2024

I think that's exactly the reason for doing it by default.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

I think that's exactly the reason for doing it by default.

Not sure I understand this. What do you mean as "by default"? I think the original PR adding -c option to pip-compile might just have missed the fact that URL is supported by pip.

@webknjaz
Copy link
Member

webknjaz commented Jan 2, 2024

I'm not sure pip-tools should support everything that pip does the same way. Its purpose is to aid reproducibility and constraints that are mutable at random points in time aren't exactly that.

@webknjaz
Copy link
Member

webknjaz commented Jan 2, 2024

Whether it was overlooked is something that only the original PR author knows, I think.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

I'm not sure pip-tools should support everything that pip does the same way. It's purpose is to aid reproducibility and constraints that are mutable at random points in time aren't exactly that.

The thing is, pip-compile already supports this.

In requirements.in:

-c https://example.com/some/constraints.txt

foo
bar

pip-compile ... requirements.in

There is no reason to not to support it via command line option I think.

@honnix
Copy link
Contributor Author

honnix commented Jan 2, 2024

In terms of reproducibility, I think it boils down to how the remote constraints file is served and digested. A local file does not ensure reproducibility either.

@honnix
Copy link
Contributor Author

honnix commented Jan 3, 2024

I'm not exactly sure why CI / Linters / linkcheck-docs (pull_request) failed but it doesn't seem to be related to the change.

@honnix
Copy link
Contributor Author

honnix commented Jan 22, 2024

Please let me know how I can move this forward together with #2040. Thank you.

@webknjaz
Copy link
Member

I'm not exactly sure why CI / Linters / linkcheck-docs (pull_request) failed but it doesn't seem to be related to the change.

Try rebasing. It's probably fixed on main.

As for the change, I guess it's okay to make the CLI behave the same way as what's in the input files.

@honnix
Copy link
Contributor Author

honnix commented Mar 4, 2024

Please help merge this when suitable. Thank you all for the reviews and suggestions.

@chrysle chrysle added this pull request to the merge queue Mar 4, 2024
Merged via the queue into jazzband:main with commit 1197151 Mar 4, 2024
36 checks passed
@chrysle
Copy link
Contributor

chrysle commented Mar 4, 2024

Thank you!

@atugushev atugushev added the bug Something is not working label Mar 5, 2024
@atugushev atugushev added this to the 7.4.1 milestone Mar 5, 2024
@honnix honnix deleted the patch-1 branch March 8, 2024 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants