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

Fix pyproject.toml for setuptools >= 69.0.0 #153

Closed
wants to merge 14 commits into from

Conversation

ranchodeluxe
Copy link
Collaborator

@ranchodeluxe ranchodeluxe commented Nov 26, 2023

Python's packaging disaster makes me want to switch languages

Addresses: #152

@ranchodeluxe ranchodeluxe added test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test. labels Nov 26, 2023
Copy link

codecov bot commented Nov 26, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (95ce6f4) 96.36% compared to head (a9cefc7) 96.36%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #153   +/-   ##
=======================================
  Coverage   96.36%   96.36%           
=======================================
  Files          15       15           
  Lines         495      495           
=======================================
  Hits          477      477           
  Misses         18       18           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cisaacstern cisaacstern left a comment

Choose a reason for hiding this comment

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

@ranchodeluxe apologies for the delay here somehow I missed the initial ping.

I am not able to reproduce the error you describe in #152, and therefore pending further discussion am not convinced this PR is needed.

I just:

  • Created an empty python 3.10 venv
  • Upgraded setuptools to 69.0.2
  • Checked out pangeo-forge-runner main
  • Ran pip install -e . without error

Beyond this experiment, I am a bit confused why the release note you quoted in #152

Added strict enforcement for project.dynamic in pyproject.toml. This removes the transitional ability of users configuring certain parameters via setup.py without making the necessary changes to pyproject.toml (as mandated by PEP 612). (pypa/setuptools#4066)

...would affect us here, given that we don't have a setup.py?

Totally open to the possibility that I'm missing something here.

@ranchodeluxe
Copy link
Collaborator Author

@ranchodeluxe apologies for the delay here somehow I missed the initial ping.

I am not able to reproduce the error you describe in #152, and therefore pending further discussion am not convinced this PR is needed.

I just:

* Created an empty python 3.10 venv

* Upgraded setuptools to `69.0.2`

* Checked out pangeo-forge-runner `main`

* Ran `pip install -e .` without error

Beyond this experiment, I am a bit confused why the release note you quoted in #152

Added strict enforcement for project.dynamic in pyproject.toml. This removes the transitional ability of users configuring certain parameters via setup.py without making the necessary changes to pyproject.toml (as mandated by PEP 612). (pypa/setuptools#4066)

...would affect us here, given that we don't have a setup.py?

Totally open to the possibility that I'm missing something here.

Yeah, not sure. I might be missing something too b/c it works for me now as well but was definitely blocking me until I had this fix in. I think closing it is fine.

Somehow I got here through the maze of links that started here maybe: https://stackoverflow.com/questions/77523055/missingdynamic-license-defined-outside-of-pyproject-toml-is-ignored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-dataflow Add this label to PRs to trigger Dataflow integration test. test-flink Add this label to PRs to trigger Dataflow integration test.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants