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

Support options in requirements.txt in pip-sync #824

Merged
merged 5 commits into from
Mar 4, 2020

Conversation

atugushev
Copy link
Member

@atugushev atugushev commented May 23, 2019

Adds support of the following options in requirements.txt in pip-sync:

--index-url
--extra-index-url
--no-index
--find-links
--no-binary
--only-binary
--trusted-host

Closes #637
Closes #638
Inspired by #703 #464.

Changelog-friendly one-liner: Support requirements.txt options in pip-sync

Contributor checklist
  • Provided the tests for the changes.
  • Requested a review from another contributor.
  • Gave a clear one-line description in the PR (that the maintainers can add to CHANGELOG.md on release).
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #824 into master will increase coverage by 0.21%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #824      +/-   ##
==========================================
+ Coverage   99.26%   99.47%   +0.21%     
==========================================
  Files          34       34              
  Lines        2446     2494      +48     
  Branches      306      312       +6     
==========================================
+ Hits         2428     2481      +53     
+ Misses         10        6       -4     
+ Partials        8        7       -1     
Impacted Files Coverage Δ
tests/conftest.py 100.00% <0.00%> (ø) ⬆️
tests/test_cache.py 100.00% <0.00%> (ø) ⬆️
piptools/resolver.py 100.00% <0.00%> (ø) ⬆️
piptools/scripts/compile.py 100.00% <0.00%> (ø) ⬆️
tests/test_repository_pypi.py 100.00% <0.00%> (ø) ⬆️
piptools/repositories/pypi.py 95.16% <0.00%> (+2.48%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d405bf9...e927939. Read the comment docs.

@atugushev atugushev added the enhancement Improvements to functionality label May 23, 2019
@atugushev atugushev added this to the 3.8.0 milestone May 23, 2019
@jazzband jazzband deleted a comment from codecov bot May 23, 2019
@atugushev atugushev removed this from the 3.8.0 milestone Jun 6, 2019
@atugushev atugushev requested a review from vphilippon June 6, 2019 18:46
@jnns
Copy link

jnns commented Aug 22, 2019

Thank you for taking care of this. 👍

We hit that problem in one of our projects. Can we help in any way to get this fix in with the next version?

@blueyed
Copy link
Contributor

blueyed commented Aug 22, 2019

@jnns
Review and test it? :)

@atugushev
Copy link
Member Author

Hi, @jnns! Thanks for being interesting in this PR. I'll rebase this branch to the master then (for there was a lot of changes since this PR has been created), let's see whether the tests pass.

@atugushev atugushev force-pushed the pip-sync-infile-options branch 2 times, most recently from a3c7c1f to 890d1b5 Compare August 25, 2019 00:32
@atugushev atugushev closed this Aug 26, 2019
@atugushev atugushev reopened this Aug 26, 2019
@atugushev
Copy link
Member Author

atugushev commented Aug 26, 2019

@jnns

We hit that problem in one of our projects. Can we help in any way to get this fix in with the next version?

FIY PR is ready to test and review. Take a look at it please if you have time.

@jnns
Copy link

jnns commented Aug 27, 2019

@atugushev Thank you! Your branch fixes the problem we have with --extra-index-url. We can now sync our requirements without pip-sync bailing out.

@atugushev
Copy link
Member Author

@jnns great! Let's wait a review from contributors then.

["--only-binary", ":all:"],
],
)
@mock.patch("piptools.sync.check_call")
Copy link
Member

Choose a reason for hiding this comment

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

FTR the preferred way of patching things in pytest is to use a built-in fixture called monkeypatch.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are there any links to the discussion about monkeypatch vs mock? I see only pytest-dev/pytest#4576 and a few others where arguments are mostly based on personal preferences. Personally, I do like @mock.patch decorator.

Would you like to open an issue so that we can discuss it and decide whether it's worth to refactor codebase form mock to monkeypatch?

Copy link
Member

Choose a reason for hiding this comment

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

I just think that it's cleaner because it's natively integrated + fewer deps.
Also, it automatically, unpatches things on exit.

Copy link
Member Author

@atugushev atugushev Oct 21, 2019

Choose a reason for hiding this comment

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

fewer deps

MagicMock would require pytest-mock btw.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's vice versa

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure.. but that's not monkeypatch.

Copy link
Member

Choose a reason for hiding this comment

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

Wait, the patching method may still be monkeypatch while the value would be MagicMock().

Copy link
Member Author

@atugushev atugushev Oct 27, 2019

Choose a reason for hiding this comment

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

@webknjaz

I just think that it's cleaner because it's natively integrated + fewer deps.

You are right, it could be the mock.MagicMock. Instead of using mock and patch things with mock, the suggestion is to use monkeypatch with mock.MagicMock. I don't get it, how it can be "fewer deps"?

Copy link
Member

Choose a reason for hiding this comment

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

It's fewer deps under py3. Also, I'm not sure I'd actually endorse magic mock, better define a proper fake object which would fit expectations instead of implicitly hiding possibly malicious behaviors.

Copy link
Member Author

@atugushev atugushev Oct 27, 2019

Choose a reason for hiding this comment

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

I see. Pretend might be an alternative to the MagickMock, btw.

piptools/scripts/sync.py Outdated Show resolved Hide resolved
atugushev and others added 4 commits February 11, 2020 01:01
Reduce the nesting.

Co-Authored-By: Sviatoslav Sydorenko <[email protected]>
Rename _build_install_flags() to _compose_install_flags()
and update the docstring.

Co-Authored-By: Sviatoslav Sydorenko <[email protected]>
@atugushev
Copy link
Member Author

@Nebukadneza I've rebased the PR against current master. Thanks for pinging!

@amonsch
Copy link

amonsch commented Feb 15, 2020

bump

Copy link
Member

@codingjoe codingjoe left a comment

Choose a reason for hiding this comment

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

I'd probably go for --binary and --source and drop the "only" or "no" prefixes.

@altendky
Copy link

How would that work given that pip uses the prefixes? I didn't think we were creating new options, just supporting existing pip ones.

$ venv/bin/pip --version
pip 20.0.2 from /home/altendky/venv/lib/python3.8/site-packages/pip (python 3.8)
$ venv/bin/pip install --help
<snip>
  --no-binary <format_control>
                              Do not use binary packages. Can be supplied multiple times, and each time adds to the
                              existing value. Accepts either :all: to disable all binary packages, :none: to empty the
                              set, or one or more package names with commas between them (no colons). Note that some
                              packages are tricky to compile and may fail to install when this option is used on them.
  --only-binary <format_control>
                              Do not use source packages. Can be supplied multiple times, and each time adds to the
                              existing value. Accepts either :all: to disable all source packages, :none: to empty the
                              set, or one or more package names with commas between them. Packages without binary
                              distributions will fail to install when this option is used on them.
  --prefer-binary             Prefer older binary packages over newer source packages.
<snip>

@Nebukadneza
Copy link

ACK with @altendky, I think it’s common convention and supported behavior pip to put the standard options of pip in requirements.txt. I think this is what users would expect to get in requirement.in too. Another downside of inventing custom options would be that users need a custom translation layer between the 2 worlds …

@amonsch
Copy link

amonsch commented Feb 24, 2020

bump

@JensPfeifle
Copy link

Would be interested in getting this merged as well. Any help needed?

@atugushev
Copy link
Member Author

Hello @JensPfeifle,

This PR needs an approvement form one of the members.

@auvipy auvipy merged commit c4dcaf7 into jazzband:master Mar 4, 2020
@auvipy
Copy link
Contributor

auvipy commented Mar 4, 2020

thanks all for your great work!

@atugushev
Copy link
Member Author

@auvipy thanks for reviewing and merging. Looks like this PR should be rebased onto master before merging. Master's CI is broken now.

@webknjaz
Copy link
Member

webknjaz commented Mar 4, 2020

@atugushev that could be solved by https://bors.tech/

@altendky
Copy link

altendky commented Mar 4, 2020

GitHub can also just require that PRs be up to date.

@atugushev
Copy link
Member Author

@altendky

GitHub can also just require that PRs be up to date.

Sounds good to me!

@atugushev
Copy link
Member Author

Addressed in #1085.

@atugushev
Copy link
Member Author

I made a post-merge conflict fix in #1086. Please, review.

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

Successfully merging this pull request may close these issues.

pip-sync ignores options in requirements.txt pip-sync ignores option --no-binary
10 participants