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

Update to v0.22.0 #156

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Update to v0.22.0 #156

merged 2 commits into from
Aug 9, 2023

Conversation

xylar
Copy link
Contributor

@xylar xylar commented Aug 5, 2023

Checklist

closes #155

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@xylar
Copy link
Contributor Author

xylar commented Aug 5, 2023

@conda-forge-admin, please rerender

@xylar
Copy link
Contributor Author

xylar commented Aug 5, 2023

I would appreciate at least one more set of eyes on:
https://github.com/SciTools/cartopy/blob/v0.22.0/pyproject.toml
to make sure I didn't mess anything up.

It would also be worth discussing if we want to make cartopy-with-* packages with any of the optional dependencies:
https://github.com/SciTools/cartopy/blob/v0.22.0/pyproject.toml#L52-L56
My current inclination is not to do that unless there is demand but it wouldn't be hard.

recipe/meta.yaml Outdated
Comment on lines 8 to 9
url: https://pypi.io/packages/source/C/Cartopy/Cartopy-{{ version }}.tar.gz
sha256: 89d5649712c8582231c6e11825a04c85f6f0cee94dbb89e4db23eabca1cc250a
url: https://github.com/SciTools/cartopy/archive/refs/tags/v{{ version }}.tar.gz
sha256: 7adf6b92f4e16c46fb1ce58840275af7a6dc1f21d5243d4b665ecef72961f9f4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Source is no longer being included on PyPI as discussed in #155

@xylar
Copy link
Contributor Author

xylar commented Aug 5, 2023

      LookupError: setuptools-scm was unable to detect version for /home/conda/feedstock_root/build_artifacts/cartopy_1691225778495/work.
      
      Make sure you're either building from a fully intact git repository or PyPI tarballs. Most other sources (such as GitHub's tarballs, a git checkout without the .git folder) don't contain the necessary metadata and will not work.
      
      For example, if you're using pip, instead of https://github.com/user/proj/archive/master.zip use git+https://github.com/user/proj.git#egg=proj
      [end of output]

I will try to remind myself what the workaround for this is but in the meantime I've submitted an issue upstream:
SciTools/cartopy#2228

Comment on lines -40 to -41
- scipy >=0.10
- ucrt # [win]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Scipy is only included as an extra in this version. I'm not sure what ucrt was needed for but I didn't see any mention of it in this release.

Copy link
Member

Choose a reason for hiding this comment

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

It is a universal C runtime and it was added in #116 (comment)

The CIs are passing so I'm OK leaving it out. Hopefully we won't break cartopy for Windows users :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zklaus, you were the one to recommend adding this library. It seemed to make CI pass at the time but doesn't seem to be needed now. I didn't find any mention of it (outside of people listing their environments) upstream: https://github.com/search?q=repo%3ASciTools%2Fcartopy+ucrt&type=issues

Do you think it's okay to remove it at this point?

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC, @zklaus is on holiday this month

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, yes. In that case maybe we assume the passing tests are a sign that this is no longer needed? It seemed like the PR where this was added indicated that the Win tests only passed after it was added.

Copy link
Member

Choose a reason for hiding this comment

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

I say merge and we can always add it back if someone reports it broken. Better to get this out there.

Copy link
Member

Choose a reason for hiding this comment

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

I'll add that cartopy isn't doing anything special here on building (just a bit of Cython at this point), so if it is somehow needed, it really should be something else exporting the requirement.

@xylar
Copy link
Contributor Author

xylar commented Aug 5, 2023

@conda-forge/cartopy, this is ready for review.

@xylar
Copy link
Contributor Author

xylar commented Aug 6, 2023

A question is whether we're okay with switching to GitHub for the source as done here or if we want to wait for source to be uploaded to PyPI, see SciTools/cartopy#2228 (comment)

@ocefpaf
Copy link
Member

ocefpaf commented Aug 7, 2023

A question is whether we're okay with switching to GitHub for the source as done here or if we want to wait for source to be uploaded to PyPI, see SciTools/cartopy#2228 (comment)

I'm OK moving to GH and even moving back to PyPI after this release. Whatever you feel is easier. The only advantage to PyPI releases is that, some times, they are slimmer than the GH counterparts b/c authors remove some of the "fat."

@xylar
Copy link
Contributor Author

xylar commented Aug 7, 2023

@ocefpaf, I also feel like bypassing setuptools-scm isn't very clean so I would prefer to see if the source gets pushed to PyPI in the next day or 2 as it seems it might.

@xylar xylar marked this pull request as draft August 7, 2023 12:24
@dopplershift
Copy link
Member

I just uploaded the sdist to PyPI.

@xylar
Copy link
Contributor Author

xylar commented Aug 8, 2023

Thanks @dopplershift!

@xylar xylar marked this pull request as ready for review August 8, 2023 07:41
@xylar
Copy link
Contributor Author

xylar commented Aug 8, 2023

@conda-forge/cartopy, I have moved the source back to pypi and this is again ready for review (assuming tests pass).

@dopplershift dopplershift merged commit c36a9f5 into conda-forge:main Aug 9, 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.

regro-cf-autotick-bot erroring with v0.22 update
4 participants