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

Use the commit date for build metadata #3322

Closed
wants to merge 2 commits into from
Closed

Conversation

codebien
Copy link
Contributor

@codebien codebien commented Sep 5, 2023

What?

Use the date of the latest commit in the working tree during the build process.

Note: GitHub uses the COMMIT DATE and not the AUTHOR DATE as you see in git log classic dump.

Why?

Previously, the date of the build was generated at runtime blocking the possibility of reproducible builds.

Related PR(s)/Issue(s)

#1996

Use the date of the latest commit in the working tree during the build
process. Previously, the date of the build was generated at runtime
blocking the possibility to have reproducible builds.
@codebien codebien added this to the v0.47.0 milestone Sep 5, 2023
@codebien codebien self-assigned this Sep 5, 2023
@codecov-commenter
Copy link

codecov-commenter commented Sep 5, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.01% 🎉

Comparison is base (bec53bf) 73.20% compared to head (07b68e1) 73.21%.

❗ Current head 07b68e1 differs from pull request most recent head 732549d. Consider uploading reports for the commit 732549d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3322      +/-   ##
==========================================
+ Coverage   73.20%   73.21%   +0.01%     
==========================================
  Files         258      258              
  Lines       19895    19895              
==========================================
+ Hits        14564    14566       +2     
+ Misses       4406     4405       -1     
+ Partials      925      924       -1     
Flag Coverage Δ
ubuntu 73.14% <ø> (+0.01%) ⬆️
windows 73.05% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@codebien
Copy link
Contributor Author

codebien commented Sep 6, 2023

It seems to work as you can see from the Build check:

Building platform: linux-amd64 ( GOOS=linux GOARCH=amd64 CGO_ENABLED=0 go build -o dist/k6-v0.46.0-31-gc68b379b-linux-amd64/k6 -trimpath -ldflags \
-X go.k6.io/k6/lib/consts.VersionDetails=2023-09-05T17:42:51+0000/v0.46.0-31-gc68b379b )

I've rebuilt it again today to confirm that the date doesn't change. The 2nd run: https://github.com/grafana/k6/actions/runs/6087972451/job/16536142053

@codebien codebien marked this pull request as ready for review September 6, 2023 08:26
build-release.sh Outdated Show resolved Hide resolved
olegbespalov
olegbespalov previously approved these changes Sep 6, 2023
@codebien
Copy link
Contributor Author

codebien commented Sep 6, 2023

The version printed by git describe --tags --always --long --dirty on my machine is different from what I see printend in the action. 🤔

@olegbespalov If you fetch this branch what do you get running the command?

@olegbespalov
Copy link
Contributor

git describe --tags --always --long --dirty
v0.46.0-31-g732549d7

@codebien
Copy link
Contributor Author

codebien commented Sep 6, 2023

The build stage says https://github.com/grafana/k6/actions/runs/6096099795/job/16540972908?pr=3322 + export VERSION=v0.46.0-32-g07b68e19

Any idea why? 🤔

@olegbespalov
Copy link
Contributor

olegbespalov commented Sep 6, 2023

I belive it's because it's a pull request https://stackoverflow.com/questions/68061051/get-commit-sha-in-github-actions

@codebien codebien requested review from mstoykov and removed request for oleiade September 7, 2023 08:59
build-release.sh Outdated
@@ -10,7 +10,8 @@ export OUT_DIR="${1-dist}"
export VERSION=${2:-$(git describe --tags --always --dirty)}

# To overwrite the version details, pass something as the third arg. Empty string disables it.
export VERSION_DETAILS=${3-"$(date -u +"%FT%T%z")/$(git describe --tags --always --long --dirty)"}
export COMMIT_DATE=$(git log -1 --format=%at $VERSION | { read d ; date -u +"%FT%T%z" -d @"$d" ; })
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest ... I am kind of for dropping this 🤷

We have the commit - that is all we need. Or we have a really well tagged build - so again the date of the build doesn't really matter.

To be honest I really was hoping that we can get rid of this all together and just rely on ReadBuildInfo, unfortunately go build in practice never populates this (see here for reasons).

So in practice that is not ... possible unless we figure out how to make go install compile. Which works except that:

  1. -o does not work :(
  2. GOBIN setting does not work with GOOS and friends

So ... in theory we can have go install ...; mv $GOBIN/k6 ... somehwere

But this seems ... bad - I currently hope that the 57485 will be fixed 🤞 or go build will be allowed to buildinfo.

Currently if you do go install vs what we have as binary the output is different:

[mstoykov@fedora k6]$ go install go.k6.io/k6@v0.46.0
[mstoykov@fedora k6]$ k6 version
k6 v0.46.0 (v0.46.0, go1.21.1, linux/amd64)
[mstoykov@fedora k6]$ k6.v0.46.0 version
k6 v0.46.0 (2023-08-14T13:23:26+0000/v0.46.0-0-gcbd9e9ad, go1.20.7, linux/amd64)

And to be honest I find our build to be worse 🤷

so .. to reiterate ... can we drop the date at least?

@@ -10,7 +10,8 @@ export OUT_DIR="${1-dist}"
export VERSION=${2:-$(git describe --tags --always --dirty)}

# To overwrite the version details, pass something as the third arg. Empty string disables it.
export VERSION_DETAILS=${3-"$(date -u +"%FT%T%z")/$(git describe --tags --always --long --dirty)"}
export COMMIT_DATE=$(git log -1 --format=%ct $VERSION | { read d ; date -u +"%FT%T%z" -d @"$d" ; })
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
export COMMIT_DATE=$(git log -1 --format=%ct $VERSION | { read d ; date -u +"%FT%T%z" -d @"$d" ; })
export COMMIT_DATE=$(git log -1 --format=%ct HEAD | { read d ; date -u +"%FT%T%z" -d @"$d" ; })

I feel like this is what you meant ?

You also can just drop HEAD I guess.

@codebien
Copy link
Contributor Author

codebien commented Sep 8, 2023

Closing in favor of #3327

@codebien codebien closed this Sep 8, 2023
@codebien codebien removed this from the v0.47.0 milestone Oct 2, 2023
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