WIP python_requirements_info does not parse extra in dependencies #5802

Warkdev · 2023-01-08T21:11:11Z

SUMMARY

Fixes #731

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

python_requirements_info

ADDITIONAL INFORMATION

Fixes the issue mentionned earlier
Also allows spaces inside the dependencies (which was not the case until now), shall this be a separate PR if.. ?

ansibullbot · 2023-01-08T21:12:35Z

cc @willthames
click here for bot help

plugins/modules/python_requirements_info.py

changelogs/fragments/5802-python-requirements-info-does-not-parse-extra-syntax.yml

Co-authored-by: Felix Fontein <felix@fontein.de>

…rse-extra-syntax.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Co-authored-by: Felix Fontein <felix@fontein.de>

…arse-project-extra-syntax.yml Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Co-authored-by: Felix Fontein <felix@fontein.de>

russoz

So, second time around taking a look, I think I spotted a couple of things. I understand not all of the issues pointed will be handled in this PR, but please take a look.

plugins/modules/python_requirements_info.py

tests/integration/targets/python_requirements_info/tasks/731-parse-project-extra-syntax.yml

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

felixfontein

Quick comment (spotted when not really looking).

plugins/modules/python_requirements_info.py

Warkdev · 2023-01-14T21:25:25Z

I have highly refactored the module to leverage the pkg_resources module capabilities, removing the need for a regex.

However, after many researches, it seems impossible to handle extras, the only guessable thing seems to be if a provided extra is valid or not.

Therefore, in case extra identifiers are provided, I propose to verify if the extra is valid, if not, an error is raised. Otherwise, the package is put inside an ignore list and a warning is issued.

Warkdev · 2023-01-14T21:45:11Z

Given the failing systems, I do think that this would require even more work to be consistent.

It may also be a good idea to start changing the submodule used on more recent versions of Python as requested by setuptools: https://setuptools.pypa.io/en/latest/pkg_resources.html

plugins/modules/python_requirements_info.py

russoz · 2023-01-15T04:36:18Z

tests/integration/targets/python_requirements_info/tasks/main.yml

+- name: ensure module supports PEP-508 specification
+  python_requirements_info:
+    dependencies:
+      - SomeProject
+      - SomeProject == 1.3
+      - SomeProject >= 1.2, < 2.0
+      - SomeProject[foo, bar]
+  register: dep_info
+
+- name: ensure python_requirements_info returns desired info
+  assert:
+    that:
+    - dep_info is not failed


IMHO this makes more sense in the 731-parse-project-extra-syntax.yml file. And shouldn't we use an existing package instead of SomeProject ?

Does it ? Because it should be handled by the general module normally :)

Regarding the SomeProject, given that we assess only the validity of the requirement format, I'm not sure which added-value we could benefit there. WDYT ?

Does it ? Because it should be handled by the general module normally :)

That came from the fact that we are testing some extra deps in there, but I see what you mean. Yeah, I am OK with it as-is.

Regarding the SomeProject, given that we assess only the validity of the requirement format, I'm not sure which added-value we could benefit there. WDYT ?

Ok, fair enough. But me lazily not going through the whole thing again, are testing elsewhere that all the extras specified are present, when responding?

russoz · 2023-01-15T04:37:52Z

plugins/modules/python_requirements_info.py

+  - The requirements C(SomeProject == 5.4 ; python_version < "3.8"), C(SomeProject ; sys_platform == 'win32') are not considered valid.
+  - The version operator C(~=) is also not considered valid on every platform.


Ha, I thought pkg_resources would be able to handle those cases.

I thought also but it seems to be platform-dependant. Check the CI results, some are working, some are giving errors.

Also, it seems we should not be using pkg_resources after Python 3.8. Going to check that.

Ok, I did not know about the platform dependency. Fair enough. Any idea on how to go around pkg_resources on >3.8 ?

Yes, I further checked it but my time has been limited. According to pkg_resources documentation, we should be using the embedded importlib:

Use of pkg_resources is discouraged in favor of importlib.resources, importlib.metadata, and their backports (importlib_resources, importlib_metadata). Please consider using those libraries instead of pkg_resources.

The migration guide being here: https://importlib-metadata.readthedocs.io/en/latest/migration.html

I am only worried that this could introduce a difference in how the module behaves (especially in term of input validation) between the two libs. Also the support for importlib is not the same between 3.8 and 3.9.

A third option that I have found, but not ideal because it adds yet another library is to use this pep-508 package to parse the input: https://pypi.org/project/pep508/

This could streamline it.

Definitely, the tag easyfix on the issue isn't or we need to scope exactly what this PR should support :-)

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

felixfontein · 2024-02-23T20:02:29Z

This PR needs a rebase. Please rebase, or if you no longer want to continue it, please close it. Thanks!

needs_info

ansibullbot · 2024-03-26T20:42:43Z

@Warkdev This pullrequest is waiting for your response. Please respond or the pullrequest will be closed.

click here for bot help

Warkdev added 8 commits January 8, 2023 20:04

Add changelog

07cfb0b

Add test that match parsing failure

bdf11a3

Refine testing

b487f06

Add regular expression to catch extras in python package

039c2d8

Update doc

09367df

Make sure that dep is trimmed (regex doesn't support spaces)

fd8457b

Update test to use a package less likely available on the image

aaae100

Update PR number

9634815

ansibullbot added WIP Work in progress bug This issue/PR relates to a bug integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) system tests tests labels Jan 8, 2023

Use sample Pypi project instead of pywinrm

a9a3b37

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-5 backport-6 Automatically create a backport for the stable-6 branch labels Jan 8, 2023

This comment was marked as outdated.

Sign in to view

felixfontein reviewed Jan 9, 2023

View reviewed changes

Warkdev and others added 7 commits January 9, 2023 07:32

Update plugins/modules/python_requirements_info.py

ea4ee1c

Co-authored-by: Felix Fontein <felix@fontein.de>

Update changelogs/fragments/5802-python-requirements-info-does-not-pa…

46797cf

…rse-extra-syntax.yml Co-authored-by: Felix Fontein <felix@fontein.de>

Update plugins/modules/python_requirements_info.py

0ad6c57

Co-authored-by: Felix Fontein <felix@fontein.de>

add wrong syntax check

3cc57c3

Refine error message

a24b03e

Refine tests

6d29b1d

Make pkg contains extras if found

5c992b6

Warkdev and others added 2 commits January 10, 2023 07:30

Update tests/integration/targets/python_requirements_info/tasks/731-p…

f4c2107

…arse-project-extra-syntax.yml Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Update plugins/modules/python_requirements_info.py

9d2bf04

Co-authored-by: Felix Fontein <felix@fontein.de>

russoz reviewed Jan 11, 2023

View reviewed changes

plugins/modules/python_requirements_info.py Outdated Show resolved Hide resolved

plugins/modules/python_requirements_info.py Outdated Show resolved Hide resolved

tests/integration/targets/python_requirements_info/tasks/731-parse-project-extra-syntax.yml Outdated Show resolved Hide resolved

Warkdev and others added 3 commits January 14, 2023 13:14

Add extra test

a092a4f

Update plugins/modules/python_requirements_info.py

c4d0357

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Amend RETURN description

f20ccff

felixfontein reviewed Jan 14, 2023

View reviewed changes

plugins/modules/python_requirements_info.py Outdated Show resolved Hide resolved

ansibullbot removed the new_contributor Help guide this first time contributor label Jan 14, 2023

Refactor code to leverage the pkg_resources capabilities

559e216

Warkdev added 3 commits January 14, 2023 22:30

Update main.yml

e648d16

Remove one more PEP-508 notation

a62e32b

Update doc

36f553f

ansibullbot added the needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR label Jan 14, 2023

russoz reviewed Jan 15, 2023

View reviewed changes

Warkdev and others added 3 commits January 15, 2023 20:19

Update plugins/modules/python_requirements_info.py

7b16ef5

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Update plugins/modules/python_requirements_info.py

2c6b4d7

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Update plugins/modules/python_requirements_info.py

d3f14d8

Co-authored-by: Alexei Znamensky <103110+russoz@users.noreply.github.com>

Warkdev changed the title ~~python_requirements_info does not parse extra in dependencies~~ WIP python_requirements_info does not parse extra in dependencies Jan 18, 2023

ansibullbot added the WIP Work in progress label Jan 18, 2023

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Jan 26, 2023

felixfontein removed the backport-5 label Apr 24, 2023

felixfontein added the backport-7 Automatically create a backport for the stable-7 branch label May 10, 2023

ansibullbot added the needs_rebase https://docs.ansible.com/ansible/devel/dev_guide/developing_rebasing.html label Jun 19, 2023

ansibullbot added the needs_info This issue requires further information. Please answer any outstanding questions label Feb 23, 2024

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI and removed stale_ci CI is older than 7 days, rerun before merging labels Mar 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP python_requirements_info does not parse extra in dependencies #5802

WIP python_requirements_info does not parse extra in dependencies #5802

Warkdev commented Jan 8, 2023

ansibullbot commented Jan 8, 2023

This comment was marked as outdated.

russoz left a comment

felixfontein left a comment

Warkdev commented Jan 14, 2023

Warkdev commented Jan 14, 2023

russoz Jan 15, 2023

Warkdev Jan 15, 2023

russoz Jan 18, 2023

russoz Jan 15, 2023

Warkdev Jan 15, 2023

russoz Jan 18, 2023

Warkdev Jan 18, 2023

felixfontein commented Feb 23, 2024

ansibullbot commented Mar 26, 2024

		- The requirements C(SomeProject == 5.4 ; python_version < "3.8"), C(SomeProject ; sys_platform == 'win32') are not considered valid.
		- The version operator C(~=) is also not considered valid on every platform.

WIP python_requirements_info does not parse extra in dependencies #5802

Are you sure you want to change the base?

WIP python_requirements_info does not parse extra in dependencies #5802

Conversation

Warkdev commented Jan 8, 2023

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

ansibullbot commented Jan 8, 2023

This comment was marked as outdated.

russoz left a comment

Choose a reason for hiding this comment

felixfontein left a comment

Choose a reason for hiding this comment

Warkdev commented Jan 14, 2023

Warkdev commented Jan 14, 2023

russoz Jan 15, 2023

Choose a reason for hiding this comment

Warkdev Jan 15, 2023

Choose a reason for hiding this comment

russoz Jan 18, 2023

Choose a reason for hiding this comment

russoz Jan 15, 2023

Choose a reason for hiding this comment

Warkdev Jan 15, 2023

Choose a reason for hiding this comment

russoz Jan 18, 2023

Choose a reason for hiding this comment

Warkdev Jan 18, 2023

Choose a reason for hiding this comment

felixfontein commented Feb 23, 2024

ansibullbot commented Mar 26, 2024