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

dynamic version strings #1676

Merged
merged 7 commits into from
Aug 15, 2024
Merged

dynamic version strings #1676

merged 7 commits into from
Aug 15, 2024

Conversation

pazz
Copy link
Owner

@pazz pazz commented Aug 13, 2024

version strings are now being

  • pulled from setuptools-scm at package level (pythonproject.toml), and
  • from importlib (packaged metadata) from within alot (for --version and UserAgent headers)

@pazz pazz requested a review from lucc August 13, 2024 13:36
@pazz
Copy link
Owner Author

pazz commented Aug 13, 2024

It looks like the nix CI script is hardcoding access to a now non-existing version attribute

Copy link
Collaborator

@lucc lucc left a comment

Choose a reason for hiding this comment

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

In general this looks good. Can you please activate this branch on RTD as we have to fix the conf.py file with the dynamic version. I am not sure if it is possible to install alot on rtd because we would have to install notmuch and gpg as well. Or how can we use the importlib.metadata.version function otherwise?

I will push a commit to fix the nix build with the dynamic version.

@lucc
Copy link
Collaborator

lucc commented Aug 13, 2024

If I add this change the nix flake builds (both alot and the docs). I am not sure if this change will also work on RTD because in the nix flake we can successfully install notmuch & gpg with nixpkgs. This might be difficult on RTD.

diff --git a/docs/source/conf.py b/docs/source/conf.py
index 98761a23..35ab9ed7 100644
--- a/docs/source/conf.py
+++ b/docs/source/conf.py
@@ -2,6 +2,7 @@
 # alot documentation build configuration file
 import pathlib
 import tomllib
+import importlib.metadata

 pyproject = pathlib.Path(__file__).parent.parent.parent / "pyproject.toml"
 with pyproject.open("rb") as f:
@@ -54,9 +55,9 @@ copyright = "Copyright (C) 2012-24 Patrick Totzke"
 # built documents.
 #
 # The short X.Y version.
-version = project_data["version"]
+version = importlib.metadata.version("alot")
 # The full version, including alpha/beta/rc tags.
-release = project_data["version"]
+release = importlib.metadata.version("alot")

 # The language for content autogenerated by Sphinx. Refer to documentation
 # for a list of supported languages.

@lucc
Copy link
Collaborator

lucc commented Aug 13, 2024

Maybe something like in eeb6a87 can also be done on RTD. This is the hack we had on travis some years ago.

flake.nix Outdated
@@ -19,12 +19,14 @@
dependencies' = pkgs.lib.lists.concatMap (builtins.match "([^>=<]*).*") pyproject.project.dependencies;
# the package is called gpg on PyPI but gpgme in nixpkgs
dependencies = map (x: if x == "gpg" then "gpgme" else x) dependencies';
version = "0.11.dev+${if self ? shortRev then self.shortRev else "dirty"}";
Copy link
Owner Author

Choose a reason for hiding this comment

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

this looks slightly dodgy: the hope was to eliminate explicit verstion strings (like the 0.11.dev prefix here) that need to be maintained. Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The architecture of nix has two distinct phases:

  1. the evaluation phase where the nix code is evaluated, in order for this to be deterministic we need all (flake-) dependencies in the lock file. The result is a build plan for step 2.
  2. the build phase: this happens in a sandbox without internet access, we can run all the commands and use all the files that we included in our build plan and produce some directory structure and files as a result.

The consequence of this is that we can not run arbitrary commands (e.g. python) during phase 1. We could read static files from disk from all our flake inputs and the git repo itself (because they are considered constant/deterministic).

The postfix of the version in ${if ... then ... else ...} is special nix flake related code that produces the short git hash if we build from a clean git state and the string "dirty" otherwise.

Now I also want to add a small prefix to the version string. If python/pip accepts this I could just use "dev". I have to check https://peps.python.org/pep-0440/ again. Maybe we can also use "0.dev" or something that is obviously "wrong" so people pay more attention to the git hash in the version.
I will have a look at this when I am at home again and come back to you.

PS: Strictly speaking nix does not need the version. As a package manager it does not use version constraints. All dependencies are always specified exact via git hashes or download urls+sha hashes. The version is mainly used for pip to integrate it into the code and to append it to the resulting nix-store path (the location where alot is installed on disk) for humans to see it.

Copy link
Owner Author

Choose a reason for hiding this comment

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

thanks for the elaborate response.
Then I suppose we can simply use an obviously false verstion string like "XXX" or similar?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sadly we can not use a totally bogus version as nix passes this version to pip/setuptools which results in the following error:

Creating a wheel...
* Getting build dependencies for wheel...
Traceback (most recent call last):
  File "/nix/store/dq4mc3gmvwxsqv4fms1adk7nqd082nkd-python3.11-pyproject-hooks-1.0.0/lib/python3.11/site-packages/
    main()
  File "/nix/store/dq4mc3gmvwxsqv4fms1adk7nqd082nkd-python3.11-pyproject-hooks-1.0.0/lib/python3.11/site-packages/
    json_out['return_val'] = hook(**hook_input['kwargs'])
                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/dq4mc3gmvwxsqv4fms1adk7nqd082nkd-python3.11-pyproject-hooks-1.0.0/lib/python3.11/site-packages/
    return hook(config_settings)
           ^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    return self._get_build_requires(config_settings, requirements=['wheel'])
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    self.run_setup()
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    exec(code, locals())
  File "<string>", line 1, in <module>
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    return distutils.core.setup(**attrs)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    _setup_distribution = dist = klass(attrs)
                                 ^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    self.metadata.version = self._normalize_version(self.metadata.version)
                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    normalized = str(Version(version))
                     ^^^^^^^^^^^^^^^^
  File "/nix/store/4yilrk744k6x8kw2gmjbqx9jyx6zc2ak-python3.11-setuptools-69.2.0/lib/python3.11/site-packages/setu
    raise InvalidVersion(f"Invalid version: '{version}'")
setuptools.extern.packaging.version.InvalidVersion: Invalid version: 'dev+09285db'

ERROR Backend subprocess exited when trying to invoke get_requires_for_build_wheel

Same for dev+dirty :(

It works with a "0dev" and "0.dev" prefix though. I guess that is valid according to pep 440. I would prefer "0.dev" as a prefix, are you fine with that?

@lucc
Copy link
Collaborator

lucc commented Aug 14, 2024

@pazz it seems that readthedocs does not pick up new commits in the rtd branch any longer. Can you please activate build for dversion (or pull requests in general)? I suspect that we are still missing #1676 (comment)

Patrick Totzke and others added 3 commits August 14, 2024 22:56
.. dynamically and remove some other metadata from alot/__init__.py to
avoid duplication

This is used in `alot -v` but also when setting `UserAgent` for
outgoing mails
@lucc
Copy link
Collaborator

lucc commented Aug 14, 2024

I took the liberty to rebase, add the diff from #1676 (comment) and force push this branch.

The build process on readthedocs seems to use some strange parsing logic
because the command `sed -i '/gpg/d;/notmuch2/d' pyproject.toml`
resulted in an error:
~~~
sed: no input files
/bin/sh: 1: /notmuch2/d: not found
~~~
@lucc
Copy link
Collaborator

lucc commented Aug 14, 2024

@pazz pazz merged commit 6ed00a6 into master Aug 15, 2024
24 checks passed
@pazz pazz deleted the dversion branch August 15, 2024 08:58
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