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

Addopt setuptools-scm without vendoring setuptools-scm #4537

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

abravalheri
Copy link
Contributor

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

@abravalheri abravalheri force-pushed the workaround-setuptools-scm branch 2 times, most recently from e00d179 to 85c28f2 Compare August 2, 2024 10:05
if: contains(matrix.python, 'pypy')
run: echo "SETUPTOOLS_ENFORCE_DEPRECATION=0" >> $GITHUB_ENV
- name: Install tox
run: python -m pip install tox
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just moved this part around such that tox would be installed before pre-building distributions1.

Footnotes

  1. The pre-built distributions were previously introduced to be used with virtualenv fixtures, it is also used by downstream packagers.

@@ -172,6 +175,8 @@ jobs:
timeout-minutes: 75
steps:
- uses: actions/checkout@v4
with:
fetch-depth: 0
Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

@abravalheri abravalheri Aug 13, 2024

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.

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])
Copy link
Contributor Author

@abravalheri abravalheri Aug 2, 2024

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()

Copy link
Member

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.

@abravalheri abravalheri changed the title [WIP] Addopt setuptools-scm without vendoring setuptools-scm Addopt setuptools-scm without vendoring setuptools-scm Aug 2, 2024
@abravalheri
Copy link
Contributor Author

@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.

@abravalheri abravalheri marked this pull request as ready for review August 2, 2024 10:35
@abravalheri abravalheri requested a review from jaraco August 2, 2024 10:35
.stable Outdated
@@ -0,0 +1 @@
v72.1.0
Copy link
Contributor Author

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").

Copy link
Member

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").

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.

Copy link
Contributor Author

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).

Copy link
Member

@jaraco jaraco left a 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',
Copy link
Member

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.

Copy link
Contributor Author

@abravalheri abravalheri Aug 13, 2024

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
Copy link
Member

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").

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
Comment on lines 29 to 35
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()


Copy link
Member

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.

Copy link
Contributor Author

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.
Copy link
Member

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().

Copy link
Contributor Author

@abravalheri abravalheri Aug 13, 2024

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...

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])
Copy link
Member

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
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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.

- name: Pre-build distributions for test
shell: bash
run: |
rm -rf dist
# workaround for pypa/setuptools#4333
tox -e version
Copy link
Member

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).

Copy link
Contributor Author

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).

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.

2 participants