-
Notifications
You must be signed in to change notification settings - Fork 10
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
Unpin versioningit version #196
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #196 +/- ##
=======================================
Coverage 97.61% 97.61%
=======================================
Files 17 17
Lines 1760 1760
=======================================
Hits 1718 1718
Misses 42 42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@@ -1,6 +1,6 @@ | |||
[build-system] | |||
# Setuptools version should match setup.py; wheel because pip will insert it noisily | |||
requires = ["setuptools >= 42.0.0", "versioningit ~= 0.3.0", "wheel"] | |||
requires = ["setuptools >= 42.0.0", "versioningit", "wheel"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be
requires = ["setuptools >= 42.0.0", "versioningit", "wheel"] | |
requires = ["setuptools >= 42.0.0", "versioningit >= 0.3.0", "wheel"] |
then so some pip env with outdated version would not just silently proceed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Projects with [build-system]
tables should always be built in isolated environments with freshly-installed dependencies (e.g., via build). A "pip env with an outdated version" should have no effect.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then we should not have versioning limit on setuptools
either (consistency is good) and why did we get it there in the first place?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
>= 42.0.0
just sets a lower bound (as opposed to ~= 0.3.0
, which also sets an upper bound), so it's harmless.
why did we get it there in the first place?
Copy & paste from an earlier era. When setuptools v42.0.0 was new, I believe it was possible for isolated build environments to be created with an older setuptools version automatically installed (because venv
and virtualenv
both bundle setuptools instead of downloading the latest version every time an environment is created), so specifying a minimum setuptools version forced an update if the environment's version was older.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think that specifying meinimal compatible version when known is a matter of good habits etc, but since it is not generally followed (since typically we do not know what is a minimal version) -- I will not insist here. Let's proceed!
825c180
to
0c3af97
Compare
versioningit
is not meant to have an upper version bound (unless you're using custom methods, which you're not), as restricting it cuts you out of bugfixes & new features. Also, the{version}
field in the "format" table is now deprecated in favor of{base_version}
(though{version}
will continue to work).