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

hack: remove -installsuffix build flag #44853

Merged
merged 4 commits into from
Jan 20, 2023

Conversation

crazy-max
Copy link
Member

follow-up #44546 (comment)

- What I did

-installsuffix netgo has been added in #10155 to work around a bug with "go build" but not required anymore since go 1.5: golang/go@4dab6d0

Also took the opportunity to:

  • remove ORIG_BUILDFLAGS var that was used for the cross target but has been removed
    in Dockerfile: use TARGETPLATFORM to build Docker #44546 so not necessary anymore
  • cleanup unnecessary vars in make.sh script
  • display build cmd when DOCKER_DEBUG is set (had to remove extra tabs and new lines to avoid a clunky output)

- How I did it

- How to verify it

- Description for the changelog

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

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.

Code changes look good to me (left one minor comment).

Also looked at the commit messages, and I know referring to PR numbers can be "noisy"; I generally use commits instead (which also allows finding back the related change without GitHub). Perhaps you could change those?

hack: remove -installsuffix build flag

Has been introduced in #10155
to work around a bug with "go build" but not required anymore

Can you change the PR number to 232d59baeb13778abc242a602ca434d83e1eb6e8 (commit?)

hack: remove ORIG_BUILDFLAGS var
This var was used for the cross target but it has been removed
in #44546 so not necessary anymore

Can you change the PR number to a commit? (Is there a specific commit we can point to? Otherwise, we could use the merge commit; b9fe30dad49db41fc8fb41da8fef46f0fb0d42b9

-X \"github.com/docker/docker/dockerversion.BuildTime=${BUILDTIME}\" \
-X \"github.com/docker/docker/dockerversion.PlatformName=${PLATFORM}\" \
-X \"github.com/docker/docker/dockerversion.ProductName=${PRODUCT}\" \
-X \"github.com/docker/docker/dockerversion.DefaultProductLicense=${DEFAULT_PRODUCT_LICENSE}\" "
Copy link
Member

Choose a reason for hiding this comment

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

Could be worth keeping the last quote on a separate line (it's easy to miss that it's there); probably that was one of the reasons to use the indentation here.

Has been introduced in 232d59b to work around a bug with
"go build" but not required anymore since go 1.5: golang/go@4dab6d0

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
This var was used for the cross target but it has been removed
in 8086f40 so not necessary anymore

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
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

@thaJeztah
Copy link
Member

Let me bring this one in. I added a cherry-pick label to keep the 23 branch a bit cleaned up as well, but it's definitely not an urgent one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants