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

properly determine go versions #3251

Merged
merged 1 commit into from
Jun 1, 2023

Conversation

deitch
Copy link
Contributor

@deitch deitch commented May 31, 2023

This extracts the unique pkg/pillar/scripts/getversion.sh and replaces it with tools/goversion.sh, which calculates the actual version for a go package using the go semver + pseudo-version rules. Unfortunately, no standard go tool does this for you.

getversion.sh was being used only to set -X main.Version=<version> for pillar. That version was interesting, and didn't actually confirm to go rules.

This now uses the standard version at a higher level, and then includes it in the 3 go binaries we build: edge-view, newlog, pillar.

The process is as follows:

  1. In Makefile, have every eve-<pkg> target depend upon pkg/%/gopkgversion
  2. In Makefile, the pkg/%/gopkgversion generates the version from goversion.sh and saves it in the package in pkg/%/gopkgversion
  3. *gopkgversion has been added to .gitignore
  4. In the Dockerfiles and/or Makefiles that build binaries, the ldflag -X main.Version=<version> has been updated to source from that file.

The gopkgversion non-tracked file allows us to build in subdirs like pkg/pillar/ or pkg/newlog/ without requiring the entire .git directory in the context.

@deitch deitch requested review from eriknordmark and rvs as code owners May 31, 2023 08:57
@deitch deitch force-pushed the proper-semver-binaries branch from 1242df6 to 3af81f5 Compare May 31, 2023 09:17
@@ -6,7 +6,7 @@ RUN eve-alpine-deploy.sh
COPY ./ /newlog/.
WORKDIR /newlog

RUN GO111MODULE=on CGO_ENABLED=0 go build -ldflags "-s -w" -mod=vendor -o /out/usr/bin/newlogd ./cmd
RUN GO111MODULE=on CGO_ENABLED=0 go build -ldflags "-s -w -X=main.Version=$(cat gopkgversion)" -mod=vendor -o /out/usr/bin/newlogd ./cmd
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RUN GO111MODULE=on CGO_ENABLED=0 go build -ldflags "-s -w -X=main.Version=$(cat gopkgversion)" -mod=vendor -o /out/usr/bin/newlogd ./cmd
RUN GO111MODULE=on CGO_ENABLED=0 go build -ldflags "-s -w -X=main.Version=$(<gopkgversion)" -mod=vendor -o /out/usr/bin/newlogd ./cmd

Copy link
Contributor

Choose a reason for hiding this comment

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

or is that a bashism?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a bashism. I originally had that, and spent easily 20-30 mins trying to figure out why the ldflag was blank. If I had submitted it right away, yetus would have told me, but I insisted on figuring it out myself first. 😆

tools/goversion.sh Outdated Show resolved Hide resolved
Signed-off-by: Avi Deitcher <avi@deitcher.net>
@deitch deitch force-pushed the proper-semver-binaries branch from 3af81f5 to d320ff6 Compare May 31, 2023 15:37
Copy link
Contributor

@eriknordmark eriknordmark left a comment

Choose a reason for hiding this comment

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

LGTM

I wonder if we take this opportunity to do the eve-version in similar ways e.g., by having the Makefile drop the ROOTFS_VERSION into ./eve-version and then use that. But this can be a separate PR.

@deitch
Copy link
Contributor Author

deitch commented May 31, 2023

I am happy to get a PR in for that.

We also were thinking of doing that for kernel builds, so we can avoid some of the complex templating we do. I will try top get to a PR on that too.

@eriknordmark eriknordmark merged commit c6a45fe into lf-edge:master Jun 1, 2023
@deitch deitch deleted the proper-semver-binaries branch June 1, 2023 07:46
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.

4 participants