-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Addopt setuptools-scm
without vendoring setuptools-scm
#4537
base: main
Are you sure you want to change the base?
Conversation
e00d179
to
85c28f2
Compare
if: contains(matrix.python, 'pypy') | ||
run: echo "SETUPTOOLS_ENFORCE_DEPRECATION=0" >> $GITHUB_ENV | ||
- name: Install tox | ||
run: python -m pip install tox |
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.
@@ -172,6 +175,8 @@ jobs: | |||
timeout-minutes: 75 | |||
steps: | |||
- uses: actions/checkout@v4 | |||
with: | |||
fetch-depth: 0 |
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.
setuptools-scm
fails without the history/tags in the repository.
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 wonder why this isn't a problem for other projects I use that employ setuptools_scm. It's a little frustrating that this concern needs to be addressed in multiple different places. Probably it should instead reference a common setting that documents its purpose. No need to deal with that yet, until we decide to go forward with this approach.
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.
Probably because setuptools
history is long? I am not sure... But I did find this issue in other projects using setuptools-scm
before and also in other CIs (like CirrusCI and the old Travis) that similarly perform a shallow clone by default.
tools/finalize.py
Outdated
towncrier.check_changes() | ||
update_changelog() | ||
bump_version() | ||
subprocess.check_call(['git', 'commit', '-a', '-m', f'Finalize #{version}']) | ||
subprocess.check_call(['git', 'tag', '-a', '-m', '', version]) |
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.
We probably can simplify this based on #4532 (comment).
But since this is only a preliminary draft, I did not look to much into it yet. May be improved in the future.
A possible approach to investigate would be:
import pathlib
import subprocess
from jaraco.develop import towncrier
from jaraco.develop.finalize import finalize
version = towncrier.semver(towncrier.get_version())
pathlib.Path(".latest").unlink() # Remove "unstable"/development version
pathlib.Path(".stable").write_text(version, encoding="utf-8")
subprocess.check_output(['git', 'add', ".stable"])
finalize()
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 might even say that we should provide hooks in jaraco.develop to implement these behaviors.
setuptools-scm
without vendoring setuptools-scm
setuptools-scm
without vendoring setuptools-scm
@jaraco this is a draft on the idea in #4530 (comment). If you agree with the basic idea, I can work to further improve it. |
.stable
Outdated
@@ -0,0 +1 @@ | |||
v72.1.0 |
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.
Instead of having this file, I can try to work on something like writing directly to setuptools/version.py
... But thought this was going to be the easiest approach.
This file helps to support fallback for workflows that don't use tox
and/or sdist
(e.g. downstream or when testing unpublished versions with pip install "setuptools @ git+https://github.com/abravalheri/setuptools@workaround-setuptools-scm"
).
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.
Instead of having this file, I can try to work on something like writing directly to
setuptools/version.py
... But thought this was going to be the easiest approach.This file helps to support fallback for workflows that don't use
tox
and/orsdist
(e.g. downstream or when testing unpublished versions withpip install "setuptools @ git+https://github.com/abravalheri/setuptools@workaround-setuptools-scm"
).
One of my goals in adopting setuptools_scm is to avoid having to mutate and commit a file on every release. This info is already tracked in the repo (by way of tags). Storing it here is redundant and adds noise. In the case of the current implementation, the noise is small, because the commit is shared with the "finalize" step, but if the "render changelog" step were also somehow eliminated (a more distant goal), this behavior would still demand an extra commit with every release.
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.
Unfortunately I did not found a way of implementing this particular methodology without having a "fallback" file. If I remove it then workflows that do not use the wheel/sdist/tox
would result in a weird version of setuptools being reported (like 0.0.0
or 0.dev0+unknow
).
85c28f2
to
9aac174
Compare
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.
Thanks for this. It's nice to see what the alternative approach might be. As you can seem from my comments, this falls short of what I'm trying to achieve by adopting setuptools_scm natively. Still, I'm not giving up on it, as the alternative isn't even viable.
conftest.py
Outdated
@@ -28,6 +28,7 @@ def pytest_configure(config): | |||
|
|||
|
|||
collect_ignore = [ | |||
'tools/save_version.py', |
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'd probably include the script in the tests unless it causes problems.
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 found a CLI option for setuptools_scm
that obviates the need for this script.
.stable
Outdated
@@ -0,0 +1 @@ | |||
v72.1.0 |
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.
Instead of having this file, I can try to work on something like writing directly to
setuptools/version.py
... But thought this was going to be the easiest approach.This file helps to support fallback for workflows that don't use
tox
and/orsdist
(e.g. downstream or when testing unpublished versions withpip install "setuptools @ git+https://github.com/abravalheri/setuptools@workaround-setuptools-scm"
).
One of my goals in adopting setuptools_scm is to avoid having to mutate and commit a file on every release. This info is already tracked in the repo (by way of tags). Storing it here is redundant and adds noise. In the case of the current implementation, the noise is small, because the commit is shared with the "finalize" step, but if the "render changelog" step were also somehow eliminated (a more distant goal), this behavior would still demand an extra commit with every release.
setup.py
Outdated
def get_version() -> str: | ||
version_files = (f for f in [".latest", ".stable"] if os.path.exists(f)) | ||
with open(next(version_files), encoding="utf-8") as fp: | ||
return fp.read() | ||
|
||
|
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.
The presence of this function is really unfortunate. We're trying to push users toward declarative config and reducing logic in setup.py files, yet we have to re-implement logic to resolve versions. One of my goals in using setuptools_scm is to follow the best practice available to other users, eat our own dogfood.
Users might be tempted to model after this behavior, and that might lead to users requesting a feature in setuptools to allow directives to load a version from any number of possible files.
Ideally this need would be systematized so it's not managed by bespoke software unique to setuptools.
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.
The way I see, the setup.py
file is an inevitable part of the ecosystem, and probably it is here to stay (not as a CLI tool, but as a Python configuration file like many others, e.g. conftest.py
, noxfile.py
, etc...).
If not setup.py
then we would end up with something very similar and "imperative"/"programatic". For example, hatch
docs suggest a hatch_build.py script as means of configuration. Granted it uses a different API and requires a class definition rather than calling a setup()
function, but at that point, I don't think it is worthy re-inventing a new API, since setuptools.setup()
is already well known.
I think setup.py
files are useful for 2 reasons:
- It is very practical for users to implement customisations as part of the local directory.
- Avoid the need for defining a full-blown plugin when the customisation is particular to a single project and the logic does not generalise well.
In this case, I think that the logic "get the content from the first of 2 files to exist" sounds very particular to this implementation, therefore setup.py
sounds like the proper place to write it.
@@ -1,30 +1,24 @@ | |||
""" | |||
Finalize the repo for a release. Invokes towncrier and bumpversion. | |||
Finalize the repo for a release. Invokes towncrier. |
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 was really hoping to see this file gone, as it mostly duplicates jaraco.develop.finalize()
.
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 have removed most of the duplication based on the approach discussed in #4537 (comment).
We could add support for a hook in jaraco.develop.finalize
, as suggested in #4537 (comment), for example:
# Potential change in jaraco.develop.finalize
Hook: TypeAlias = Callable[[str], Iterable[str]]
def finalize(modify_files: Hook = lambda _: []):
...
if modified := modify_files(version):
subprocess.run(["git", "add", *modified], ...)
...
Or something like a --save-version=FILE
CLI option...
tools/finalize.py
Outdated
towncrier.check_changes() | ||
update_changelog() | ||
bump_version() | ||
subprocess.check_call(['git', 'commit', '-a', '-m', f'Finalize #{version}']) | ||
subprocess.check_call(['git', 'tag', '-a', '-m', '', version]) |
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 might even say that we should provide hooks in jaraco.develop to implement these behaviors.
MANIFEST.in
Outdated
@@ -18,4 +18,5 @@ include pytest.ini | |||
include tox.ini | |||
include setuptools/tests/config/setupcfg_examples.txt | |||
include setuptools/config/*.schema.json | |||
include .latest .stable |
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'm slightly disinclined to use .
-prefixed filenames. They're platform sensitive (visible by default on Windows) and IMO are just a crutch. Moreover, I'd like to move ancillary concerns out of the root.
What do you think instead about (meta)/latest.txt
and (meta)/stable.txt
?
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 have updated the PR to reflect that suggestion.
@@ -172,6 +175,8 @@ jobs: | |||
timeout-minutes: 75 | |||
steps: | |||
- uses: actions/checkout@v4 | |||
with: | |||
fetch-depth: 0 |
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 wonder why this isn't a problem for other projects I use that employ setuptools_scm. It's a little frustrating that this concern needs to be addressed in multiple different places. Probably it should instead reference a common setting that documents its purpose. No need to deal with that yet, until we decide to go forward with this approach.
.github/workflows/main.yml
Outdated
- name: Pre-build distributions for test | ||
shell: bash | ||
run: | | ||
rm -rf dist | ||
# workaround for pypa/setuptools#4333 | ||
tox -e version |
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.
Surely this new command isn't meant to be a workaround for the mentioned issue; probably the command should appear before that comment. It would be nice if this command didn't need to happen at all (if it happened as part of running tox / installing the source tree).
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 have changed the order of the lines so that this problem does not happen.
In theory we could remove it, as it would not be a problem for the tests, but I think it is nice to have a consistent naming of the tested artifacts, so for now, I prefer to leave it there (which might change after more investigation).
9aac174
to
561f959
Compare
Summary of changes
This is an attempt on implementing the approach described in #4530 (comment).
The main idea is to use
setuptools-scm
outside of the PyPA build flow and cache the output into files.Closes
Pull Request Checklist
newsfragments/
.(See documentation for details)