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

Set DAEMON_VERSION from git metadata #30

Merged
merged 15 commits into from
Oct 6, 2024

Conversation

git-developer
Copy link

@git-developer git-developer commented Sep 22, 2024

This is a proposal for #14 that reads the DAEMON_VERSION from git metadata and replaces the value of the constant at build time.

With this approach, the constant in constants.py is not versioned in git. It could be changed to something like dev, latest, local or the like.

When the GIT_DESCRIPTION build-arg is set, the daemon version is determined automatically. Alternatively, the build-arg DAEMON_VERSION may be set explicitely (merely for debugging and local experiments).

Examples

Git Ref Git Description DAEMON_VERSION
v0.4.9.2 v0.4.9.2 0.4.9.2
v0.4.9.2^ v0.4.9.1.1-8-g9f7dc8b 0.4.9.1.1.8.dev167238795
images v0.4.8.16-2-g414fc45 0.4.8.16.2.dev68484165

References

@git-developer
Copy link
Author

Idea: maybe the version constant can be externalized to an .env file so that constants.py is not touched.

@C0rn3j
Copy link
Owner

C0rn3j commented Sep 23, 2024

So basically, it will only remain broken for building from git sources directly(where we could somehow set version from git tag instead, ideally)?

The releases will have AppImages with correct versions, and sources with the correct versions too as per their release tags?

EDIT: Ah it does not touch the release files -> could it?
Then we could have it fixed everywhere without bumping constants.
We could have some soft-error check in the CI that will warn about the version being out of date in the repo for those.

Or perhaps use tags for source builds if it matches the latest, and git hash for source builds where there's no direct tag for the commit.

@git-developer
Copy link
Author

So basically, it will only remain broken for building from git sources directly(where we could somehow set version from git tag instead, ideally)?

When the build is triggered via Docker (no matter if on GitHub CI or locally), the DAEMON_VERSION can be set automatically. Local command would be

$ docker build -o build-output --build-arg "GIT_DESCRIPTION=$(git describe --tags)" .

If you prefer some other hard-coded version, you can use --build-arg DAEMON_VERSION=... instead.
So nothing should "remain broken". If you build without docker, no replacement is done (so behavior depends on the value in constants.py).

The releases will have AppImages with correct versions, and sources with the correct versions too as per their release tags?

I'm a little bit confused by the terms "releases", "versions" and "sources". The version of a GitHub Release is taken from a git tag. The version of an AppImage is also derived from git, it just adds a classifier for the distro. The source code is tagged in Git. So yes, everything matches and Git is the only location where the version is managed.

EDIT: Ah it does not touch the release files -> could it? Then we could have it fixed everywhere without bumping constants. We could have some soft-error check in the CI that will warn about the version being out of date in the repo for those.

Sorry, I don't understand. There should be no chance for anything to get out-of-date.

Or perhaps use tags for source builds if it matches the latest, and git hash for source builds where there's no direct tag for the commit.

The git describe command applies this logic. When you build and the git HEAD points to a tag, the tag is used as version. When HEAD does not point to a tag, a new version is calculated from the last tag and the number of commits since that one.

Do these explanations help a little bit? Do you have further questions or see problems with this approach? Does the code contain any other version numbers besides the DAEMON_VERSION?

@C0rn3j
Copy link
Owner

C0rn3j commented Sep 28, 2024

Does the code contain any other version numbers besides the DAEMON_VERSION?

I have just mostly migrated from the deprecated setup.py to pyproject.toml (and threw ruff.toml into it as a bonus).

This let me fix the forever-somewhat-broken About menu, but it does mean that we have a new addition in the form of [project]->version.

Unsure how to deal with it and at the moment am exhausted, will give the rest of your comment a proper response later.

EDIT: This has predictably broken the entire CI, which should now probably build by means of python -m build --wheel instead of python setup.py with the addition of some new dependencies. python-toml is needed for tests, but I fixed that already. python-poetry or its dependencies might be needed to actually run the following build commands.

python -m build --wheel and python -m installer --destdir="${pkgdir}" dist/*.whl seemed to have do the trick for my AUR packages.

EDIT2: Siiigh, it's so broken, especially the scripts. I'll have to fix it later by implementing proper imports, atm it just all loads scc with no arguments.

EDIT3: Okay, that works.

@git-developer
Copy link
Author

It seems a lot of things are involved now. I'll focus on the version here.

I think that we should have a single location that contains the version. Currently, this is constants.py. Maybe it's possible to have a dedicated file that contains nothing but the version, and that is used to fill DAEMON_VERSION?

Regarding the build setup changes: It seems to be possible to use the version from git by setting dynamic = ["version"] in the toml file without hard-coding it. Maybe that is an option?

@git-developer
Copy link
Author

git-developer commented Sep 29, 2024

Discussion on build failure moved to #36.

@C0rn3j
Copy link
Owner

C0rn3j commented Oct 4, 2024

Thaaat was the wrong number.

@C0rn3j
Copy link
Owner

C0rn3j commented Oct 4, 2024

So, with recent changes, we have version in constants.py AND pyproject.toml.
It definitely does not make sense to have both, and it makes more sense to keep it in pyproject.toml, and as dynamic.

We have these use cases:

  • run.sh -> Build env in its own venv - until last version it ran itself from the source files
  • Docker -> Build env
  • AppImages
    • Ran from Git CI -> Build env
    • Ran from scripts/appimage-AppRun.sh -> Does this even work at all? It looks to hardcode a bunch of old versions.
    • Ran from appimage-build.sh -> Same question
  • Distro packages
    • Built from a stable tag/release commit
    • Built from latest git master or otherwise not a direct tag/release

I see you've covered pretty much everything already.

I would change the versioning though, current one it not clear as to what commit it is:

v0.4.9.1.1-8-g9f7dc8b
>>>
0.4.9.1.1.8.dev167238795

I would suggest one of these formats instead if it's an out-of-tag version, so we get to keep the exact commit hash:
v0.4.9.1.1-8-g9f7dc8b
>>>
v0.4.9.1.1.dev8.g9f7dc8b
v0.4.9.1.1.8.g9f7dc8b

@git-developer
Copy link
Author

Use cases are simpler IMHO. AppImage just calls Docker and doesn't need anything (uses Git Metadata already). scripts/appimage-AppRun.sh and appimage-build.sh are both obsolete, coming from the pre-AppImageBuilder era. When TOML can be configured for dynamic, there's only constants.py remaining that needs the special handling from this PR.

it not clear as to what commit it is

Have a look at the code: the dev number is hex-to-dec converted from the git hash. Looks ugly in my eyes, but is a 1:1 representation of the git hash. Your supposed version schemes unfortunately are not compatible to PEP 440 and will not be accepted by tools as version number for a python package. See the link from my first post for the spec. In short, only dotted numbers are valid. That's why I hacked the hash-to-number conversion into it. But after some thinking about it, I now think the conversion has no benefit: it should be OK to drop the hash and just suffix .dev1 or so.

@C0rn3j
Copy link
Owner

C0rn3j commented Oct 4, 2024

In short, only dotted numbers are valid

I skimmed it and my understanding was that a-Z 0-9 + dots was valid

@git-developer
Copy link
Author

Public version identifiers are separated into up to five segments:

  • Epoch segment: N!
  • Release segment: N(.N)*
  • Pre-release segment: {a|b|rc}N
  • Post-release segment: .postN
  • Development release segment: .devN

Only local versions allow letters, separated by a +

@git-developer
Copy link
Author

Python module packaging has a Version class that validates the allowed patterns, you can play with it to check.

@git-developer
Copy link
Author

BTW, I just noticed that in the upstream repo are already a bunch of 0.4.9.* pre-releases, the latest is 0.4.9.12. Maybe we should change to 0.4.10, 0.5 or 1.0.

@git-developer
Copy link
Author

Let me explain. Suppose that the last release is v0.4.8.21 and we're developing in branch python3 containing 2 commits and we have another branch fix/foo with 1 commit:

+--------------------------o
|                          ^
o-----o-----o              |
^           ^              |
v0.4.8.21   python3        fix/foo

git describe --tags produces:

  • for branch python3: v0.4.8.21-2-g1a2b3c (2 commit since the last tag + commit hash)
  • for branch fix/foo: v0.4.8.21-1-g4d5e6f (1 commit since the last tag + commit hash)

Currently implemented daemon version:

  • for branch python3: 0.4.8.21.2.dev1715004 (whereas hex 1a2b3c is dec 1715004)
  • for branch fix/foo: 0.4.8.21.1.dev5070447

@git-developer
Copy link
Author

OK, after reading the SO answers I think that v0.4.8.21-2-g1a2b3c should be converted to 0.4.8.21.2+g1a2b3c. This local number should be allowed as long as such a release is not published to a packaging repo.

The dev suffix is not appropriate because

  1. Developmental releases are ordered by their numerical component,

  2. X.dev0 is before X, not after.

@git-developer git-developer marked this pull request as draft October 5, 2024 08:06
@C0rn3j
Copy link
Owner

C0rn3j commented Oct 5, 2024

BTW, I just noticed that in the upstream repo are already a bunch of 0.4.9.* pre-releases, the latest is 0.4.9.12. Maybe we should change to 0.4.10, 0.5 or 1.0.

I am starting to think we should stop caring about upstream as the project appears pretty much dead(kozec said he does not have access to his account anymore and does not seem on caring to restore it or create a new one), and just do whatever versioning we want.

0.4.10 at minimum is a good idea to do after I am done breaking things constantly with the beta releases.

X.dev0 is before X, not after.

The number of the commit(X) will always be higher on any subsequent commit, so whatever we do after X is irrelevant, no?

OK, after reading the SO answers I think that v0.4.8.21-2-g1a2b3c should be converted to 0.4.8.21.2+g1a2b3c. This local number should be allowed as long as such a release is not published to a packaging repo.

Looks good to me!

@git-developer
Copy link
Author

The last commit removes all the manual version fiddling and delegates to setuptools-scm. It determines a version from git similiar to the ideas above and generates a file with a python variable that can be used within the code. I tried here locally, works fine. Do you agree that this is a better way to go?

This approach has one downside: setuptools must be run for the version to exist. This breaks the tests currently because I don't know how to tell them about the version. Could you have a look? Maybe the import should not be done in constants.py, or additionally somewhere in the test code?

@git-developer
Copy link
Author

I figured it out. This PR now contains the following changes:

  • Version number is calculated by setuptools-scm
  • DAEMON_VERSION is set accordingly
  • AppImage version is set accordingly
  • Python tests have been added to the Docker build
  • Python dependency email has been removed from excludes to avoid a crash when the About dialog is opened

What's left for here or seperate PR(s):

  • Remove obsolete appimage scripts
  • Check if we really need 2 GitHub builds

@git-developer git-developer marked this pull request as ready for review October 6, 2024 09:06
Dockerfile Show resolved Hide resolved
@git-developer
Copy link
Author

Focal AppImage does not show About dialog:

Traceback (most recent call last):
  File "/tmp/.mount_sc-conef5HTv/usr/lib/python3.8/site-packages/scc/gui/app.py", line 870, in on_mnuAbout_activate
    AboutDialog(self).show(self.window)
  File "/tmp/.mount_sc-conef5HTv/usr/lib/python3.8/site-packages/scc/gui/aboutdialog.py", line 12, in __init__
    self.setup_widgets()
  File "/tmp/.mount_sc-conef5HTv/usr/lib/python3.8/site-packages/scc/gui/aboutdialog.py", line 30, in setup_widgets
    sccontroller_module = importlib.resources.files("scc")
AttributeError: module 'importlib.resources' has no attribute 'files'

importlib.resources.files was added in Python 3.9. Can we workaround this?

All other AppImages work fine.

@C0rn3j
Copy link
Owner

C0rn3j commented Oct 6, 2024

Is there anything we get out of supporting focal going forward, outside of having to keep multiple focal-specific workarounds?

If I recall correctly, your AppImage testing has shown that everything that works on focal also works on jammy, with that in mind, considering Python 3.8 is also EOL at the end of this month, we could simply drop distributions that do not ship 3.9+ (which I believe is only focal as bullseye is 3.9) and bump minimum Python req to 3.9.

Other than that, we could do some awful ImportError trycatch.

@git-developer
Copy link
Author

I agree that it's not worth to put efforts into supporting non-critical features for a distro that is EOL soon. So maybe it's best to leave the About dialog broken in Focal and prepare a PR for removal of Focal in 04/25.

@C0rn3j
Copy link
Owner

C0rn3j commented Oct 6, 2024

Let's deal with the rest of things elsewhere, thanks a lot for implementing this!

@C0rn3j C0rn3j merged commit ae6593c into C0rn3j:python3 Oct 6, 2024
1 check passed
@git-developer git-developer deleted the feature/daemon-version branch October 6, 2024 11:31
@C0rn3j
Copy link
Owner

C0rn3j commented Oct 6, 2024

I have ended up bumping the minimum Python version to 3.9 in the pyproject.toml and README.
It should not change much of anything, as all of the annoying typing changes we keep working around are only implemented in 3.10.

focal images were kept with a warning in the release notes about the broken About dialogue.

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