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

nightly: use exact branch name in $BUILDVERSION_EXEC #175

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dancrossnyc
Copy link
Contributor

No description provided.

@dancrossnyc
Copy link
Contributor Author

I ran into seemingly odd behavior where I was on the stlouis branch, but the branch name in the build was another, different branch that happened to be at that same commit. This was surprising, but seems to be the standard behavior of git describe. Adding the --match argument with the (current) branch name gives the desired behavior, but breaks if one is disconnected from any branch (e.g., one is in "detached HEAD" state as if one checks out some commit that's not associated with a ref head in the current repo), hence the "||" command to revert to the default behavior.

Perhaps there's some simpler way to do this, but if so, I'm not sure what it is.

Copy link
Contributor

@citrus-it citrus-it left a comment

Choose a reason for hiding this comment

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

I've seen this before but didn't look too closely, so thanks for investigating.
I had a quick look and couldn't find a better way to do this, which was a bit of a surprise given its clunkiness.

env += "export BUILDVERSION_EXEC=\"git describe --all --long --dirty\"\n";
env += "export BUILDVERSION_EXEC=\
\"git describe --all --long --dirty \
--match $(git branch --show-current) --exact-match 2>/dev/null || \
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this being evaluated at the time the .env file is sourced, or when BUILDVERSION_EXEC is used? It should be the latter but this looks like the former, which is a problem if you switch branches while in a bldenv.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL; it was actually borked (but it takes a while in a build to get the point where one notices). It's evaluated when BUILDVERSION_EXEC is used, but that caused syntax errors, so I've changed it to creating a function that does the branch ... stuff ... and then BUILDVERSION_EXEC invokes that function.

Ugh. Yeah, this feels way uglier than it needs to be.

@dancrossnyc dancrossnyc force-pushed the branchname branch 3 times, most recently from 3e24cc6 to 5d6c71c Compare September 9, 2024 20:08
@jclulow
Copy link
Collaborator

jclulow commented Sep 9, 2024

I think given how complex this is, that I'd like to get it done in Rust. We should have a new (hidden) subcommand for helios-build that produces the correct value and then use that in the generated environment file.

@dancrossnyc
Copy link
Contributor Author

I think given how complex this is, that I'd like to get it done in Rust. We should have a new (hidden) subcommand for helios-build that produces the correct value and then use that in the generated environment file.

I agree. This is also hopelessly broken; the issue is that the way that BUILDVERSION_EXEC is actually used is in a Makefile in usr/src/cmd/nsadmin, but it's just expanded as an unevaluated string, which cannot handle the ||. The "fix" would be to eval it in the Makefile, but that's just a bigger change and introduces its own issues. All of my hacky attempts to work around it have come to naught for what are (in retrospect) blindingly obvious reasons.

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