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.

Sign up for GitHub

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

Filter pip environment markers #647

Merged
merged 1 commit into from
Apr 10, 2018
Merged

Filter pip environment markers #647

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions piptools/scripts/compile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,10 @@ def cli(verbose, dry_run, pre, rebuild, find_links, index_url, extra_index_url,
constraints.extend(parse_requirements(
src_file, finder=repository.finder, session=repository.session, options=pip_options))

# Filter out pip environment markers which do not match (PEP496)
constraints = [req for req in constraints
if req.markers is None or req.markers.evaluate()]

# Check the given base set of constraints first
Resolver.check_constraints(constraints)

Expand Down
19 changes: 19 additions & 0 deletions tests/test_cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,3 +347,22 @@ def test_generate_hashes_with_editable():
).format(small_fake_package_url)
assert out.exit_code == 0
assert expected in out.output


def test_filter_pip_markes():
"""
Check that pip-compile works with pip environment markers (PEP496)
"""
runner = CliRunner()
with runner.isolated_filesystem():
with open('requirements', 'w') as req_in:
req_in.write(
"six==1.10.0\n"
"unknown_package==0.1; python_version == '1'")

out = runner.invoke(cli, ['-n', 'requirements'])

assert out.exit_code == 0
assert '--output-file requirements.txt' in out.output
assert 'six==1.10.0' in out.output
assert 'unknown_package' not in out.output
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for tackling this one!

Unfortunately, that implementation breaks the current behavior of keeping the markers in the requirements.txt which allows to make simple cases compatible cross-environment. See #635 (comment) and the replies afterward.

What could be done would be to make the filtering inside the Resolver, maybe in check_constraints or somewhere that applies only to the top-level constraints, so that it only excludes them from the dependency resolving part, which is were the issue currently comes from.

While I was originally discussing the removal of the markers in the requirements.txt in the comment I linked, I'm now inclined to keep them, because:

  • It keeps the simple cases of (ex: functools32 and enum34) working cross-environment
  • It keeps a form of information in the requirements.txt that will indicate that there was originally a marker, and that will help with offering support if someone puts a maker on a package with dependencies and can't figure out why some sub-dependencies aren't included in the requirements.txt.