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

Add executable mkosi.version support for generating the version dynamically #2954

Merged
merged 2 commits into from
Aug 20, 2024

Conversation

NekkoDroid
Copy link
Contributor

@NekkoDroid NekkoDroid commented Aug 14, 2024

The script is executed unsandboxed (not a fan of this, but unsure how this should be done before any configuration even exists) and reads stdout to figure out the version.

First commit allows more configurable handling of "standard config paths". Maybe this change should also be extended to mkosi.rootpw to allow generating it dynamically?

Closes #2915

@behrmann
Copy link
Contributor

Having both mkosi.version and mkosi.version.run leaves the path open to reusing the former as input for the latter, as I described earlier, but if we decide we don't need that, this could be conveniently folded into one and use mkosi.version as a script if it's executable and as the regular mkosi.version otherwise.

The script is executed unsandboxed (not a fan of this, but unsure how this should be done before any configuration even exists) and reads stdout to figure out the version.

Maybe that's not necessary? Could we run it just before the configure script?

@NekkoDroid
Copy link
Contributor Author

Having both mkosi.version and mkosi.version.run leaves the path open to reusing the former as input for the latter, as I described earlier, but if we decide we don't need that, this could be conveniently folded into one and use mkosi.version as a script if it's executable and as the regular mkosi.version otherwise.

I can fold them into one mkosi.version, was writing a separate version of this patch that did just that.

Maybe that's not necessary? Could we run it just before the configure script?

I theoretically might be possible, just it would be a bit bigger of a change since ImageVersion= is inherited to the subimage configs, which can override it if they want to and to my knowledge the %v specifier is expanded also during config parsing.

@behrmann
Copy link
Contributor

I theoretically might be possible, just it would be a bit bigger of a change since ImageVersion= is inherited to the subimage configs, which can override it if they want to and to my knowledge the %v specifier is expanded also during config parsing.

That's true, though thinking about it, that's a problem we already have with configure scripts, since they could potentially change the version. That's probably not a solvable problem, since expansions can't be unmade.

So, yes, for least surprise having the version as early as possible seems reasonable.

@NekkoDroid
Copy link
Contributor Author

NekkoDroid commented Aug 14, 2024

To be fair, this isn't the first user script that is run unsandboxed on the host system, the same is done in config_parse_dict.

mkosi/mkosi/config.py

Lines 899 to 902 in d53f31b

if os.access(p, os.X_OK):
new[p.name] = run([p], stdout=subprocess.PIPE, env=os.environ).stdout
else:
new[p.name] = p.read_text()

Should I change those lines to use config_path_run_or_read, to have them all use the same codepath? That would remove any trailing newlines though.

@NekkoDroid NekkoDroid changed the title Add executable mkosi.version.run support for generating the version dynamically Add executable mkosi.version support for generating the version dynamically Aug 14, 2024
@NekkoDroid
Copy link
Contributor Author

Pushed the cleanup commit from #2905 since I had to add the error handling for when mkosi.version is executable anyway :)

tests/test_config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
@NekkoDroid NekkoDroid force-pushed the executable-mkosi-version branch 8 times, most recently from 03b1667 to 566db05 Compare August 15, 2024 20:20
@NekkoDroid
Copy link
Contributor Author

The CI failures seem unrelated?

@behrmann
Copy link
Contributor

I'm travelling over the weekend. Will have a closer look when I have time, Monday at the latest.

The CI failures for Fedora at least are unrelated. We have issues with the Fedora keys about once every release. Haven't looked at the Arch failures.

Copy link
Contributor

@behrmann behrmann left a comment

Choose a reason for hiding this comment

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

Haven't tested it yet, but looks reasonable to me. A few last nits

mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
tests/test_config.py Show resolved Hide resolved
@NekkoDroid NekkoDroid force-pushed the executable-mkosi-version branch 3 times, most recently from 371a698 to 44fc5a8 Compare August 19, 2024 14:08
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/resources/mkosi.md Outdated Show resolved Hide resolved
@NekkoDroid NekkoDroid force-pushed the executable-mkosi-version branch 2 times, most recently from 6270f44 to 8dc5771 Compare August 20, 2024 09:30
`mkosi.version` is executed during configuration parsing, as opposed
to reading the contents of `mkosi.version`. This allows querying the
version before the build without needing to manually adjust the version
beforehand.

This allows using date based versioning by writing a script outputting
`date '+%Y-%m-%d'` or using git tag based versioning by outputting
`git describe --tags`.
@DaanDeMeyer DaanDeMeyer merged commit dd3ea85 into systemd:main Aug 20, 2024
26 of 32 checks passed
@NekkoDroid NekkoDroid deleted the executable-mkosi-version branch August 20, 2024 10:47
@septatrix
Copy link
Contributor

Thanks for implementing this Nekko 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add an executable version of mkosi.version
4 participants