Skip to content

remove legacy build-tags#5904

Merged
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:cleanup_buildtags
Mar 10, 2025
Merged

remove legacy build-tags#5904
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:cleanup_buildtags

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

- Human readable description for the release notes

- A picture of a cute animal (not mandatory but encouraged)

@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Mar 8, 2025
@thaJeztah thaJeztah requested a review from a team as a code owner March 8, 2025 11:42
@codecov-commenter

codecov-commenter commented Mar 8, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.26%. Comparing base (2eec746) to head (f6d49e9).
Report is 3 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #5904   +/-   ##
=======================================
  Coverage   59.26%   59.26%           
=======================================
  Files         357      357           
  Lines       29771    29771           
=======================================
  Hits        17645    17645           
  Misses      11153    11153           
  Partials      973      973           
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the cleanup_buildtags branch from b1cb710 to f6d49e9 Compare March 8, 2025 11:47
@thaJeztah thaJeztah changed the title remove legacy and redundant build-tags remove legacy build-tags Mar 8, 2025
@@ -1,5 +1,4 @@
//go:build unix
// +build unix
//go:build !windows

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

!windows is a lot broader than just unix. The package used here is golang.org/x/sys/unix which I assume supports all unix systems, but it's unclear to me if !windows would be too broad for it, wdyt?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In this case we mostly care about complementing the _windows file, so it's fine.

If someone attempts to build the CLI for such platform, I don't think it makes much difference if they would get an TerminationSignals is undefined vs package unix doesn't provide SIGTERM/SIGINT.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I originally removed the build-tag, but forgot that unix is "special" in Go, because for backward-compatibility, they didn't add support for _unix.go as suffix (as they do for other build-tags, like _windows.go.

I can add a signals_other.go or signals_unsupported.go instead that defines an empty slice instead?

var TerminationSignals = []os.Signal{}

or

var TerminationSignals []os.Signal

@Benehiko let me know what you prefer 👍

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine as is :)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Ah, I also just realised that this is an internal package inside cmd/docker, so really only for building the binary, and not to be used by anyone else, so probably should be fine yes.

@thaJeztah thaJeztah merged commit 7ec69de into docker:master Mar 10, 2025
@thaJeztah thaJeztah deleted the cleanup_buildtags branch March 10, 2025 10:42
@thaJeztah thaJeztah added this to the 28.0.2 milestone Apr 7, 2025
@thaJeztah thaJeztah self-assigned this May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants