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

make trimpath optional #3908

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Conversation

deitch
Copy link
Contributor

@deitch deitch commented Jun 20, 2023

Fixes #3906

Makes -trimpath optional by calling it as

make runc TRIMPATH=

The default mode is unchanged.

Comments in the Makefile make it clear that this may still be required for reproducible builds.

cc @cyphar

@deitch
Copy link
Contributor Author

deitch commented Jun 20, 2023

codespell is complaining about a non-error, that isn't even part of this PR 🤷‍♂️

# some tools use to infer the version, in the absence of go information,
# which happens when you use `go build`.
# This enables someone to override by doing `make runc TRIMPATH= ` etc.
TRIMPATH := -trimpath
Copy link
Member

Choose a reason for hiding this comment

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

I think we might want to call this something like BUILD_FLAGS_REPRODUCIBLE= because -buildvcs=false is something we will also want to add, and it will also strip debug information that users might want (notably the commit id). I think it's fair to say that most of the build flags we will add for reproducibility are going to be related to removing debug information that is less reproducible...

Or maybe you don't care about the vcs information auto-added to Go binaries in the recent versions of Go?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see use cases both ways. My immediate preference is a flag targeted on trimpath, but if you prefer the flag as you put it, I'm fine changing it. What's your preference?

Copy link
Member

Choose a reason for hiding this comment

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

BUILD_FLAGS_REPRODUCIBLE

Can we use SOURCE_DATE_EPOCH ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What does SDE have to do with whether or not we set trimpath?

Copy link
Member

Choose a reason for hiding this comment

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

If it is set, it is expected to be reproducible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh. If set, run -trimpath, else not? That does change the default from "no special var set = trimpath" to "only if special var set = trimpath", but if that's what you want, I can change this PR. Let me know.

@kolyshkin
Copy link
Contributor

codespell is complaining about a non-error, that isn't even part of this PR man_shrugging

This is being fixed in #3907 which is waiting for another LGTM. Please ignore for now.

@deitch
Copy link
Contributor Author

deitch commented Jun 21, 2023

Cool. I'll ignore. Waiting to see if @cyphar wants me to rename it as suggested in his comment, or just leave it like this.

@deitch
Copy link
Contributor Author

deitch commented Jun 27, 2023

@cyphar and @kolyshkin what can I do to move this ahead?

@deitch deitch force-pushed the trimpath-optional branch 2 times, most recently from 81424d6 to e3fd502 Compare July 2, 2023 08:34
@deitch
Copy link
Contributor Author

deitch commented Jul 2, 2023

Rebased on main now that #3907 is in. What else can I do to move this ahead?

@deitch
Copy link
Contributor Author

deitch commented Jul 2, 2023

Also, whatever you did in #3907 works, as that check no longer is failing.

@deitch
Copy link
Contributor Author

deitch commented Jul 17, 2023

Been a few weeks; can we get a review and a potential merge, please?

@deitch
Copy link
Contributor Author

deitch commented Jul 17, 2023

I rebased to latest main

Signed-off-by: Avi Deitcher <avi@deitcher.net>
Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

still LGTM

@AkihiroSuda AkihiroSuda merged commit 8fc5be4 into opencontainers:main Jun 6, 2024
37 of 39 checks passed
@deitch deitch deleted the trimpath-optional branch June 6, 2024 07:10
@lifubang lifubang mentioned this pull request Jun 10, 2024
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.

go build -trimpath makes other arguments ignored in debug.BuildInfo
4 participants