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

pkg_resources: pass python_version to marker.evaluate() #1275

Closed
wants to merge 1 commit into from

Conversation

ignatenkobrain
Copy link

We should be passing more, but unfortunately we don't know full
environment just based on filename.

$ python2 t.py hypothesis-3.44.24-py2.7.egg-info

  • Requirement.parse('attrs>=16.0.0')
  • Requirement.parse('coverage')
  • Requirement.parse('enum34')

$ python3 t.py hypothesis-3.44.24-py2.7.egg-info

  • Requirement.parse('attrs>=16.0.0')
  • Requirement.parse('coverage')

But if pkg_resources.Distribution already knows python version it is
targeting, we could guide evaluation.

Fixes: #1274
Signed-off-by: Igor Gnatenko ignatenko@redhat.com

@ignatenkobrain
Copy link
Author

I was thinking to change evaluate_marker(), but then realized that it is kinda public API....

@benoit-pierre
Copy link
Member

I don't think this is right. The whole point of markers is to test some conditions based on the current environment, including the current Python version.

You also should not be able to end up with a package in your Python path that has an advertised Python version that does not match (like above), unless you purposely break your installation by manually editing such path / moving things around, or you found a bug in pip/easy_install.

@ignatenkobrain
Copy link
Author

So how do I get python2 requires if I use python3 to run tool which uses pkg_resources?

@benoit-pierre
Copy link
Member

I'd say you can't.

@jaraco
Copy link
Member

jaraco commented Feb 10, 2018

What’s the use case?

@benoit-pierre
Copy link
Member

Looking at the linked issue above it's for finding a python package dependencies in RPM's pythondistdeps.

@ignatenkobrain
Copy link
Author

exactly, right now we have script which is printing requires and it is always executed through python3 even for python2 metadata...

@jaraco
Copy link
Member

jaraco commented Feb 11, 2018

After reviewing this in depth, I do feel like the reported defect is a legitimate defect.

If pkg_resources is only meant to open/manage/inspect packages that were installed/intended for the current Python, then it should probably raise an exception if one tries to do what's described (i.e. create a PathMetadata for a Python 2.7 (or 3.5) package on Python 3.6). If we went that route, then one would be forced to use the relevant Python environment to inspect its packages.

I'm not aware of any formal support for this cross-platform or cross-Python functionality (do correct me if I'm mistaken), but that doesn't mean it can't gain that functionality. If setuptools wishes to support this behavior, it should do so formally, with tests and documentation (at least in code).

I also feel there are other environment considerations (such as platform, which is also tracked by the metadata and guarded by extras). If it has cross-python support, it should probably have cross-platform support. And what about other environment markers for which there isn't sufficient package metadata to detect the environment?

Would you consider expanding this PR to address these concerns (documenting the intention, testing the expectation, and consider what should happen for other markers)?

In the meantime, having just spent some time in this code, I wish to refactor it in a way that will conflict with this PR, but in a way that would allow for changes like this proposal to be easier to implement at a less complicating scope.

@jaraco
Copy link
Member

jaraco commented Feb 11, 2018

Another interpretation of what this code should do is not to filter based on environment markers at all. In fact, looking at the code, I think the current filtering based on markers should probably happen in requires or between _dep_map and requires. With that kind of refactoring, it would enable a consumer like t.py to retrieve the raw dependency map and use its own logic for evaluating the markers.

@ignatenkobrain
Copy link
Author

And what about other environment markers for which there isn't sufficient package metadata to detect the environment?

We just ignore them (aka keep current behavior). Because I don't think there is way how to find those things.

Would you consider expanding this PR to address these concerns (documenting the intention, testing the expectation, and consider what should happen for other markers)?

Sure!

With that kind of refactoring, it would enable a consumer like t.py to retrieve the raw dependency map and use its own logic for evaluating the markers.

This definitely makes a lot of sense and this probably would be best thing to do for us in RPM.

Consumers of pkg_resources could use dist._dep_map to get raw data
(including environment markers) and do what they want.

It's only one place in extras where we don't use safe_extra(), so it
shouldn't break any other valid cases.

Signed-off-by: Igor Gnatenko <ignatenko@redhat.com>
@ignatenkobrain
Copy link
Author

I pushed first commit which added ability to not evaluate markers, will work on rest soon.

@jaraco jaraco added the draft label Mar 17, 2018
@arekm
Copy link

arekm commented Sep 27, 2018

How this ended in redhat python deps evaluation script since no progress on this pull request. Was this handled differently?

Found platform.python_version() mocking suggestion and that worked for me.

@jaraco
Copy link
Member

jaraco commented Sep 11, 2019

Thanks for the effort on this, but I think ultimately, pkg_resources won't implement this functionality. Instead, consumers who wish to perform specialized dependency management scenarios should rely on packaging and importlib.metadata to inspect environments and packages.

@jaraco jaraco closed this Sep 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pkg_resources use system python version instead of py_version for requires()
4 participants