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

Remove use of setuptools_scm_git_archive #2057

Merged
merged 1 commit into from Jun 28, 2022

Conversation

dopplershift
Copy link
Contributor

The .git_archival.txt in the sdist is breaking installation with new versions of pip (version comes up 0.0). Support for git archives is baked into setuptools_scm itself now with version 7.

Fixes #2053 using the removal of setuptools_scm_gitarchive identified as the root cause there.

The .git_archival.txt in the sdist is breaking installation with new
versions of pip (version comes up 0.0). Support for git archives is
baked into setuptools_scm itself now with version 7.
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

LGTM if it all builds

@dopplershift
Copy link
Contributor Author

Looks like we have some unrelated image test failures. I'm trying to get a PR together to fix that separately. That and #2056 should get CI green again.

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

I checked this out, built an sdist and pip install *.tar.gz and it appeared to get the proper version now, whereas before checking out the sdist pip would get 0.0 for the version. So I think this is good.

@greglucas greglucas merged commit 3098143 into SciTools:main Jun 28, 2022
@dopplershift dopplershift deleted the update-setuptools-scm branch June 28, 2022 02:40
@QuLogic QuLogic added this to the 0.20.3 milestone Jun 28, 2022
@greglucas
Copy link
Contributor

Do you want to backport this to the v0.20.x branch? I think this could be backported and then we could tag that commit as v0.20.2.post1 to get a new release out without needing to backport anything else.

dopplershift pushed a commit to dopplershift/cartopy that referenced this pull request Jun 28, 2022
Remove use of setuptools_scm_git_archive
dopplershift added a commit to dopplershift/cartopy that referenced this pull request Jun 28, 2022
Remove use of setuptools_scm_git_archive
@dopplershift
Copy link
Contributor Author

@greglucas Done in #2059.

@jules-ch
Copy link

jules-ch commented Jun 28, 2022

We should see if git archive install still works, excluding .git_archival.txt from sdist is necessarry but it might remove installation from git archive all together.

Anyway prioritizing pypi installation is right.

I think we need to file an issue in setuptools_scm aswell to at least document this or version inference priority.

Btw @keewis just saw this affects xarray sdist installation aswell.

@dopplershift
Copy link
Contributor Author

It looks like pypa/setuptools_scm#734 is adding information to the docs (readme). My guess on reading it is that we have broken the version detection when installing from git archives.

My question: does this matter to anyone? I've never bothered with this on my other projects. We currently upload proper sdists to PyPI and that's what conda-forge points to. We haven't documented anywhere that people should consider installing from GitHub's auto-generated tarballs. 🤷‍♂️ I don't want to hold up getting a fixed PyPI sdist out for this, but I'd review and merge a PR that fixes it up.

@QuLogic
Copy link
Member

QuLogic commented Jun 28, 2022

Git archives can sometimes be useful if sdists are missing tests, but fortunately, Cartopy's sdists include tests, so we don't need to use the GitHub tarballs.

@jules-ch
Copy link

@dopplershift I'm ok with this +1
I recall seeing an issue in pint regarding this hgrecco/pint#1423.
But IMO there are work arounds.

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.

Pip not installing the latest version of Cartopy
4 participants