-
Notifications
You must be signed in to change notification settings - Fork 127
Set version var during make build #651
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
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add to the PR description, what is the test case the PR fixes?
Makefile
Outdated
| @@ -1,10 +1,12 @@ | |||
| CODE_COVERAGE_REPORT_FOLDER = $(PWD)/build/test-coverage | |||
| CODE_COVERAGE_REPORT_NAME_UNIT = $(CODE_COVERAGE_REPORT_FOLDER)/coverage-unit-report | |||
| VER_PKG = github.com/elastic/elastic-package/internal/version | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's change it to:VERSION_IMPORT_PATH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The choice was intentional to reduce line length. It's not far from its use, so the context is still strong. If you would still like me to make that change given this, let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I suggest extending it as VER_PKG might be misleading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me how, but done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first guess was PKG_CONFIG as it is a Makefile.
Thanks for adjusting it.
Makefile
Outdated
| build: | ||
| go get -ldflags "-X github.com/elastic/elastic-package/internal/version.CommitHash=`git describe --always --long --dirty` -X github.com/elastic/elastic-package/internal/version.BuildTime=`date +%s`" \ | ||
| go install -ldflags \ | ||
| "-X $(VER_PKG).CommitHash=`git describe --always --long --dirty` -X $(VER_PKG).BuildTime=`date +%s` -X $(VER_PKG).Tag=`(git describe --exact-match --tags 2>/dev/null || echo '') | tr -d '\n'`" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe this line can be split in multiple lines for easier readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately this is not possible due to the interaction of make string handling and -ldflags behaviour.
This is a follow up from #648, @mtojek please take a look.
The change bakes in the current version tag during
make buildif the repo is checked out at a tagged version.Prior to this a user building
elastic-packageusingmakewould be given an executable that is unaware of its version. Nowmakewill leave an executable that renderselastic-package versionsimilar to this when the source tree is checked out at a tagged version:If not at a tagged version, no semver version is output.