Skip to content

Conversation

1ace
Copy link
Contributor

@1ace 1ace commented Sep 12, 2025

In Mesa, we want to append either an empty string if building from a release tarball, or the git commit but surrounded by parentheses and with a git- prefix.

This PR makes this possible, as can be seen in https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/37337

@1ace 1ace requested a review from jpakkane as a code owner September 12, 2025 15:18
@eli-schwartz
Copy link
Member

It's already possible to customize the command used to generate the tag string. With that in mind, how useful would you say it is to keep adding kwargs to the meson DSL to fine time the exact processing of said string?

default: "''"
since: 1.10.0
description: |
A string prepended before the output of the command.
Copy link
Member

Choose a reason for hiding this comment

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

I feel somehow like I'd prefer this to be mutually exclusive with specifying a custom command to run -- i.e. only support it when using the default VCS detection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without specifying a command you can only get git describe, right? You can't get just the git commit?
That wouldn't work for our use-case, but I don't know what other use-cases there are out there (or not out there yet because not supported).

This is all moot now that you made me realize that I can pass --format though, so I'm happy with whatever you prefer 👍

Comment on lines +1965 to +1966
prefix = kwargs.get('prefix', '')
suffix = kwargs.get('suffix', '')
Copy link
Member

Choose a reason for hiding this comment

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

When you add prefix and suffix to the kwtypes.VcsTag TypeDict you wong't need to use get here. :)

@dcbaker
Copy link
Member

dcbaker commented Sep 12, 2025

It would be nice to have a test for the fallback case to make sure that the prefix and suffix are not added.

@1ace
Copy link
Contributor Author

1ace commented Sep 13, 2025

It's already possible to customize the command used to generate the tag string. With that in mind, how useful would you say it is to keep adding kwargs to the meson DSL to fine time the exact processing of said string?

Oh, I hadn't thought of just passing the prefix & suffix in a --format="prefix %h suffix" argument, that's much easier and supported today; I'll do that for our use-case, thanks!

@1ace
Copy link
Contributor Author

1ace commented Sep 13, 2025

It would be nice to have a test for the fallback case to make sure that the prefix and suffix are not added.

Agreed; I didn't find any test that verifies the output of vcs_tag(), only that it doesn't crash; could you provide an example of how this could be done?

@1ace
Copy link
Contributor Author

1ace commented Sep 13, 2025

Given that one can format the vcs string through the command, I'm not sure this PR is actually useful anymore?

@1ace
Copy link
Contributor Author

1ace commented Sep 13, 2025

There's a bug though (stripping too much, breaks prefix/suffix that start/end with whitespaces), but #15027 fixes that :)

@1ace
Copy link
Contributor Author

1ace commented Sep 24, 2025

Closing for now, we can always reopen it if we decide we do want this :)

I'd love for the trivial bug fix #15027 to get reviewed and merged though ❤️

@1ace 1ace closed this Sep 24, 2025
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.

3 participants