-
-
Notifications
You must be signed in to change notification settings - Fork 611
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
#206: Preserve environment markers from requirements.in #460
#206: Preserve environment markers from requirements.in #460
Conversation
tests/test_writer.py
Outdated
result = writer._format_requirement( | ||
ireq, reverse_dependencies, primary_packages=['test'], | ||
marker=ireq.markers) | ||
result = result.replace('\"', '\'') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per your previous comment, I would add a comment to the source here why you're transforming the quotes. You could also make it obvious with a if sys.version_info...
for the affected Python version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will revisit this. A better solution might be to use double quotes in the environment marker -- I suspect these will remain double quotes in both 2.7 and 3.5. That would eliminate the need for the non-obvious replace()
operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have cleaned this up -- the test now uses double quotes, which work consistently on Python 2.7 and 3.5.
6288374
to
ccf545c
Compare
I think this will be good for the 1.9 release. |
Is this really something that pip-compile should do? Shouldn't the compiled requirements.txt target a single environment rather than having conditional dependencies for several different environments? (Environment = Python version, platform (linux/win/..), etc. which can be conditional with the markers)
What I'm thinking here is that, if the requirements.txt should be compiled in a way that it supports more than one environment, then the compilation should be able to collect all possible environment variations from the source dependencies and pin the versions and use markers accordingly. This will get very complicated. I thought pip-compile is about locking down the specific versions and specific packages and conditional entries in the generated file doesn't feel like it's in line with that. |
I think your question contains a misconception: When pip-compile "compiles" a requirements.txt file, the Python version and OS used to execute pip-compile has no impact on the output. The environment markers are simply passed through. Thus, the
The environment marker is not interpreted until package installation time. Thus the requirement.txt file is potentially compatible with any version of Python. This feature has proven very useful to me so far, and several others have requested it, too. The target use case is when there is a library that simply doesn't work on certain interpreters or platforms. Two examples:
Hence, an application might contain the following lines in
|
piptools/utils.py
Outdated
@@ -79,6 +79,8 @@ def format_requirement(ireq, include_specifier=True): | |||
line = str(ireq.req).lower() | |||
else: | |||
line = name_from_req(ireq.req).lower() | |||
if marker: | |||
line = '{} ; {}'.format(line, str(marker)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
str(marker)'s redundant here, format's {} does it for you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove.
tests/test_writer.py
Outdated
|
||
|
||
def test_format_requirement_environment_marker(from_line, writer): | ||
"Primary packages should not get annotated." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this comment applies?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably copy-paste. Will review and fix or remove.
Do you mean that your PR changes this somehow? At least currently it makes a big difference. Here's an example:
My point is: If you want the same requirements.txt to work for different environments, then pip-compile should collect all the possible environment markers that every package in the dependency tree has and take those into account. These env markers should then be combined and the generated file should have those env markers added. I don't know how easy that would be, but I bet generating a different requirements.txt files for each environment is much easier. |
Interesting, thanks for pointing out how the output differs. I hadn't noticed that. In spite of the behavior you point out, this PR still implements the feature in a useful way. Your example is similar to how I am using this feature in my own work. If I read the output correctly, the only difference between the two outputs is this package for Python 2.7:
To deal with this, simply add the following line to requirements.in:
With this addition, pip-tools creates a requirements.txt file that works on both Python 2 and Python 3. I do appreciate the comments and suggestions here, but I don't think they make a case against merging this PR. The comments fall into two categories:
Please, let's review this PR in the context of the original issue from 18 months ago, which several people have weighed in on and agreed is useful. Once this feature is in pip-compile, if it needs additional work to handle new requirements that arise, we can revisit it then. Note this feature has no impact on the behavior of pip-compile unless someone explicitly adds environment markers to requirements.in. So this feature solves real-world problems that have affected several people, and it's harmless if you don't want or need to use it. What's not to like? |
Actually there's also However I still agree with you @barrywhart , I think this feature can be merged as-is and improved down the line. It's not a huge burden to copy some packages with environment markers into |
I think it should be at least possible to toggle between preserving the environment markers from the in-file or to resolve them. Suppose you're using the "different file for each env" flow which I suggested and have some env markers in the in-file. Then you'd expect each output file to have different contents (e.g. requirements-py2.txt only having the py2 deps and requirement-py3.txt having py3 deps): each file having only the concrete dependencies for that env. |
How does this interact with generate-hashes option btw.? I've previously had some problems when I used a requirements.txt file with the generated hashes and tried to install those requirements with different Python version. The txt file was for Python 3.4 and I installed it with Python 3.5, so it wasn't even a major version difference. Having the same txt file used for more than one env should then probably have hashes of each env too or otherwise the install will fail to hash mismatch. Yeah... this is also another story and this feature will still be useful when not using hashes, but should the |
I'm not familiar with generate-hashes. From your description, it sounds like that option has issues or limitations already. I think I would prefer someone document the limitations of generate-hashes in a separate PR. Trying to have one option automatically disable another seems tricky and confusing. |
I wasn't suggesting anything fancy for this. Maybe something like this could work: # in scripts/compile.py
@click.option(...)
@click.option('--preserve-env-markers', ...)
def cli(verbose, ..., generate_hashes, ..., preserve_env_markers, ...):
...
if preserve_env_markers and generate_hashes:
raise raise click.BadParameter(
"--preserve-env-markes is not compatible with --generate-hashes") |
Would anybody else like to weigh in on the |
…ip-tools into support_environment_markers
Ok, I have addressed the two code review comments. |
Thanks @barrywhart ! |
Replaces #459
OutputWriter.write()
, pass along the markers fromrequirements.in
format_requirement()
-- add support for optional markersmarker
parameter toOutputWriter.write()
, pass it topiptools.util.format_requirement()