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

Upgrade setuptools-scm #1332

Merged
merged 2 commits into from
Dec 11, 2023
Merged

Conversation

jessicamack
Copy link
Member

Upgrading setuptools-scm to work with the upgrading of the package in AWX

@jessicamack jessicamack requested a review from a team as a code owner December 8, 2023 22:16
@github-actions github-actions bot added the needs_triage New item that needs to be triaged label Dec 8, 2023
@nitzmahone
Copy link
Member

nitzmahone commented Dec 8, 2023

I think we just capped the build dep versions for safety after the PyYAML Cython fiasco and knowing setuptools is going to start breaking things more aggressively too. If we're going to do a new release just to bump setuptools_scm, might as well bump setuptools up to the latest too.

Bigger picture question though: why isn't AWX vendoring a wheel instead of a source tarball? Then you don't have to care- either ship the wheel we provide on PyPI or roll your own during the AWX release process (which would run in an isolated build like it's supposed to and "just work").

@shanemcd
Copy link
Member

Bigger picture question though: why isn't AWX vendoring a wheel instead of a source tarball?

IIRC this is due to needing to vendor the source inside of our source RPM. But honestly ... the answer to this question shouldn't matter right? You also hit this error if you're simply trying to install from git with newer versions of setuptools / setuptools-scm. IMHO it's bad practice to cap these kinds of deps unless they are causing some issue.

@AlanCoding
Copy link
Member

@jessicamack could you give more information about why this is needed? What upgrade error led you down this path?

@AlanCoding
Copy link
Member

AlanCoding commented Dec 11, 2023

Link the code conflict:

https://github.com/ansible/django-ansible-base/blob/486c4d003c2982f4e817fd7cff7afa3b455554a0/pyproject.toml#L36

setuptools_scm>=8

Since these are both public projects that makes this fairly clear. Either runner changes or ansible_base changes to be compatible.

@john-westcott-iv any preference?

@shanemcd
Copy link
Member

I have a preference ... if there is no real reason why runner pins this, why make other projects cargo-cult this tech debt?

@AlanCoding
Copy link
Member

I agree with that preference, I just want to frame what the conflict is and establish why this is a priority now.

@sivel
Copy link
Member

sivel commented Dec 11, 2023

You also hit this error if you're simply trying to install from git with newer versions of setuptools / setuptools-scm

Generally speaking, you would be building in an isolated environment. pip by default does this, although the downstream RPM packaging runs with --no-build-isolation but is only building a single package at a time.

I'm not sure in what manner of building/installing would cause this issue. Answering the question of "What upgrade error led you down this path?" would be good to know. Since any source should be turned into a wheel intermediately, I'm not sure where this happens. I tested a pip install of both of these packages, with no errors. Unless for some reason they are all being pip installed simultaneously with also specifying --no-build-isolation.

Regardless of this being 2 packages "we" own, this could happen for any other number of packages. It seems like there are some assumptions that are not true, that are at a level above this problem, that we might want to address.

@sivel sivel requested a review from Shrews December 11, 2023 21:06
Copy link
Contributor

@Shrews Shrews left a comment

Choose a reason for hiding this comment

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

Tested this change and the only difference in metadata (between these versions and the current versions from devel) that I can see is that the source distro's PKG-INFO file contains these additional lines:

Requires-Dist: pexpect>=4.5
Requires-Dist: packaging
Requires-Dist: python-daemon
Requires-Dist: pyyaml
Requires-Dist: importlib-metadata<6.3,>=4.6; python_version < "3.10"

Those lines appear in both versions of the .whl package's METADATA file.

@Shrews Shrews merged commit 309baad into ansible:devel Dec 11, 2023
11 checks passed
@Shrews Shrews removed the needs_triage New item that needs to be triaged label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants