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

Removed install dependency on setuptools #483

Merged
merged 4 commits into from
Nov 4, 2022
Merged

Removed install dependency on setuptools #483

merged 4 commits into from
Nov 4, 2022

Conversation

agronholm
Copy link
Contributor

Fixes #470.

@codecov
Copy link

codecov bot commented Nov 2, 2022

Codecov Report

Base: 66.85% // Head: 68.28% // Increases project coverage by +1.43% 🎉

Coverage data is based on head (ad44125) compared to base (7b9e8e1).
Patch coverage: 66.66% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #483      +/-   ##
==========================================
+ Coverage   66.85%   68.28%   +1.43%     
==========================================
  Files          12       12              
  Lines         902      927      +25     
==========================================
+ Hits          603      633      +30     
+ Misses        299      294       -5     
Impacted Files Coverage Δ
src/wheel/cli/convert.py 38.60% <50.00%> (-0.11%) ⬇️
src/wheel/bdist_wheel.py 52.50% <69.56%> (+5.21%) ⬆️
src/wheel/metadata.py 98.38% <0.00%> (+6.45%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@agronholm
Copy link
Contributor Author

The failing tests are due to an old version of setuptools not collecting the licenses as we expect. Maybe I need to add a fallback here.

@defaultbranch
Copy link

defaultbranch commented Nov 2, 2022

Btw: Thanks!

@henryiii
Copy link
Contributor

henryiii commented Nov 3, 2022

Curious, I thought a) setup_requires was deprecated and b) setuptools explicitly says you can't list setuptools in setup_requires, it does nothing since setuptools is already reading it? Dropping the install_requires seems like a good move, though!

I still think the correct move here for ideal bootstrapping is to set the package up with flit. Or even have it build itself, perhaps? A custom backend isn't that hard1 when you already have the ability to make wheels. You can hard code the contents, and then you just have to make metadata.

Footnotes

  1. Unless you want to support editable installs. Yeah, Flit's easier and is intended for this.

@agronholm
Copy link
Contributor Author

I still think the correct move here for ideal bootstrapping is to set the package up with flit. Or even have it build itself, perhaps? A custom backend isn't that hard1 when you already have the ability to make wheels. You can hard code the contents, and then you just have to make metadata.

I got burned before trying to switch to PEP 517 builds. I'm willing to try that again with the publicapi branch, but in the 0.x series, I'm going backwards compatibility first. Many people are running incredibly old versions of tools, as evidenced by this recent bug report.

@agronholm
Copy link
Contributor Author

it does nothing since setuptools is already reading it?

Not true, the version number does matter.

@agronholm
Copy link
Contributor Author

It seems to work fine now. I would like someone to look over the changes and give me a thumbs up if it seems a-ok.

@defaultbranch
Copy link

You would have my thumbs up anyway, for fixing that; but I don't know much about Python...

looking at https://github.com/pypa/wheel/pull/483/files, from a very formal (non-semantic) perspective, changes seem to be low-invasive

except the change to license_paths from bdist_wheel adds a lot of new decision/fallback logic; I think this may cause even more maintenance issues in the future;

  • the old method was simple/stupid
  • the new method contains a magic number "setuptools_major_version >= 57"
  • the new method contains log messages (previously none); will this be helpful/serve a purpose ?

My hunch is that license_paths from bdist_wheel is a candidate either for the next cleanup/refactoring, or for future trouble;

So I think the change to license_paths from bdist_wheel may or may not be acceptable, but may need future consideration.

@agronholm
Copy link
Contributor Author

the old method was simple/stupid
the new method contains a magic number "setuptools_major_version >= 57"
the new method contains log messages (previously none); will this be helpful/serve a purpose ?

The "old" method, as you saw it, was introduced in the yanked release of v0.38.0. This PR restores backwards compatibility because it can no longer guarantee a specific setuptools version.

@agronholm
Copy link
Contributor Author

I did some more testing and the new code breaks on setuptools older than v42, but wheel 0.37.1 works with setuptools older than that. I will fix this shortly.

@agronholm
Copy link
Contributor Author

Unless someone points out any glaring issues in this PR, I'll merge it tomorrow and make a new patch release.

@agronholm agronholm merged commit 9ec2016 into main Nov 4, 2022
@agronholm agronholm deleted the fix-470 branch November 4, 2022 09:05
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.

wheel 0.38.0 causes circular dependency with setuptools
3 participants