Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Remove pre-Go 1.17 build tags #5525

Merged
merged 1 commit into from
Nov 21, 2024
Merged

Conversation

slonopotamus
Copy link
Contributor

@slonopotamus slonopotamus commented Nov 17, 2024

I believe that BuildKit cannot be built with such an old Go and there's no reason to keep these legacy tags.


The only manual change is util/archutil/generate.go. All the rest is just a result of go fix ./...

@tonistiigi
Copy link
Member

Is there a validation preventing this from coming back in future updates? What is the typical behavior in vscode etc. or how is it controlled there?

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Nov 19, 2024

Sorry, no idea, I don't use VSCode. I use GoLand. It highlights these tags as obsolete and suggests removing them.

@thaJeztah
Copy link
Member

go fix can remove these automatically; so far, I haven't found a linter for this, but you could add something similar as to what was done in runc;

@slonopotamus
Copy link
Contributor Author

slonopotamus commented Nov 20, 2024

Is there an actual problem with tags still being added? Or they are just legacy leftovers? If you really want, I can add a job similar to opencontainers/runc#4332

@crazy-max
Copy link
Member

I can add a job similar to opencontainers/runc#4332

If we want smth like this it would not be a new ci job but embedded in our sandbox like any other validation tool we are using.

@thaJeztah
Copy link
Member

I personally don't think it's a huge problem. I'm not a VSCode user, and don't know if it's adding them, but I know that my GoLand flags their presence as a warning depending on the version of Go used;

Screenshot 2024-11-20 at 09 59 59 Screenshot 2024-11-20 at 10 00 04 Screenshot 2024-11-20 at 10 01 20

@thaJeztah
Copy link
Member

I should add that the go fix could still be interesting for other reasons, but I don't think it's necessarily needed for this PR.

I do recall that when I removed them in moby (using go fix) it left some behind; not sure what happened at the time, or if it was due to build-tags / platform; moby/moby#46316

@thaJeztah
Copy link
Member

Looks like go fix left a couple behind here as well;

git grep '// +build' -- ':!vendor/'
examples/gobuild/main.go:// +build ignore
tools/tools.go:// +build tools
worker/runc/runc.go:// +build linux
worker/runc/runc_test.go:// +build linux

Signed-off-by: Marat Radchenko <marat@slonopotamus.org>
@github-actions github-actions bot added area/dependencies Pull requests that update a dependency file area/examples labels Nov 21, 2024
@slonopotamus
Copy link
Contributor Author

I should add that the go fix could still be interesting for other reasons, but I don't think it's necessarily needed for this PR.

I'd prefer to leave this out of scope of current PR.

Looks like go fix left a couple behind here as well;

Fixed

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM (non-binding)

@crazy-max crazy-max merged commit 05ea8c9 into moby:master Nov 21, 2024
95 checks passed
@slonopotamus slonopotamus deleted the legacy-build-tags branch November 21, 2024 14:38
@thompson-shaun thompson-shaun added this to the v0.18.0 milestone Nov 21, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants