Skip to content

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented Sep 25, 2021

Follow-up moby/moby#42872

- What I did

Replace windres with native Go implementation to be able to create a syso file which contains Microsoft Windows Version Information and an icon using goversioninfo.

This also fixes an issue with VERSIONINFO:

 > [linux/amd64 build 1/1] RUN --mount=ro --mount=type=cache,target=/root/.cache     --mount=from=dockercore/golang-cross:xx-sdk-extras,target=/xx-sdk,src=/xx-sdk     --mount=type=tmpfs,target=cli/winresources     xx-go --wrap &&     TARGET=/out ./scripts/build/binary &&     xx-verify $([ "static" = "static" ] && echo "--static") /out/docker:
#55 3.019 llvm-rc: Error in VERSIONINFO statement (ID 1): 
#55 3.038 FILEVERSION fields (1846769) does not fit in 16 bits.

Tool is not vendored but I can do that it if you want.

- How I did it

Remove windres references and replace with goversioninfo. Also use go:generate for better integration and add legal copyright field.

- How to verify it

docker buildx bake --set "*.platform=windows/amd64"
...
#16 [build 1/1] RUN --mount=type=bind,target=.,rw     --mount=type=cache,target=/root/.cache     --mount=from=dockercore/golang-cross:xx-sdk-extras,target=/xx-sdk,src=/xx-sdk     GO111MODULE=auto go install github.com/josephspurrier/goversioninfo/cmd/goversioninfo@v1.3.0 &&     xx-go --wrap &&     TARGET=/out ./scripts/build/binary &&     xx-verify $([ "static" = "static" ] && echo "--static") /out/docker
#16 0.368 go: downloading github.com/josephspurrier/goversioninfo v1.3.0
#16 0.826 go: downloading github.com/akavel/rsrc v0.10.2
#16 1.425 Building static docker-windows-amd64.exe
#16 1.437 {
#16 1.437   "FixedFileInfo":
#16 1.437   {
#16 1.437     "FileVersion": {
#16 1.437       "Major": 20,
#16 1.437       "Minor": 10,
#16 1.437       "Patch": 2,
#16 1.437       "Build": 0
#16 1.437     },
#16 1.437     "FileFlagsMask": "3f",
#16 1.437     "FileFlags ": "00",
#16 1.437     "FileOS": "040004",
#16 1.437     "FileType": "01",
#16 1.437     "FileSubType": "00"
#16 1.437   },
#16 1.437   "StringFileInfo":
#16 1.437   {
#16 1.437     "Comments": "",
#16 1.437     "CompanyName": "Docker, Inc.",
#16 1.437     "FileDescription": "Docker Client",
#16 1.437     "FileVersion": "20.10.2.0",
#16 1.437     "InternalName": "",
#16 1.437     "LegalCopyright": "Copyright (c) 2013-2021 Docker Inc. All rights reserved.",
#16 1.437     "LegalTrademarks": "",
#16 1.437     "OriginalFilename": "docker-windows-amd64.exe",
#16 1.437     "PrivateBuild": "",
#16 1.437     "ProductName": "Docker Client",
#16 1.437     "ProductVersion": "20.10.2-263-gdd69cb4d7a.m",
#16 1.437     "SpecialBuild": ""
#16 1.437   },
#16 1.437   "VarFileInfo":
#16 1.437   {
#16 1.437     "Translation": {
#16 1.437       "LangID": "0409",
#16 1.437       "CharsetID": "04B0"
#16 1.437     }
#16 1.437   }
#16 1.437 }
#16 1.437 + go generate -v github.com/docker/cli/cmd/docker
#16 1.453 cmd/docker/docker.go
#16 1.453 cmd/docker/docker_test.go
#16 1.453 cmd/docker/docker_windows_amd64.go
#16 1.478 + go build -o /out/docker-windows-amd64.exe -tags  --ldflags '    -w          -X "github.com/docker/cli/cli/version.GitCommit=dd69cb4d7a"     -X "github.com/docker/cli/cli/version.BuildTime=2021-09-25T20:58:28Z"     -X "github.com/docker/cli/cli/version.Version=20.10.2-263-gdd69cb4d7a.m"      ' github.com/docker/cli/cmd/docker
#16 DONE 5.0s

should produce:

image

COMPANY_NAME can be used to set the company that produced the windows binary.

- Description for the changelog

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

cc @thaJeztah

Signed-off-by: CrazyMax crazy-max@users.noreply.github.com

@crazy-max crazy-max force-pushed the goversioninfo branch 3 times, most recently from dd69cb4 to cece8be Compare September 25, 2021 21:01
@codecov-commenter
Copy link

codecov-commenter commented Sep 25, 2021

Codecov Report

Merging #3310 (79e933c) into master (f14d8e7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #3310   +/-   ##
=======================================
  Coverage   57.99%   57.99%           
=======================================
  Files         302      302           
  Lines       21764    21764           
=======================================
  Hits        12621    12621           
  Misses       8219     8219           
  Partials      924      924           

@crazy-max crazy-max marked this pull request as ready for review September 25, 2021 21:13
Copy link
Member

@mat007 mat007 left a comment

Choose a reason for hiding this comment

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

Nice!

@crazy-max
Copy link
Member Author

Also is the make.ps1 still used? I wonder if we could get rid of it in a follow-up? Same for appveyor.yml when #3234 is merged?

@crazy-max crazy-max force-pushed the goversioninfo branch 2 times, most recently from 4bf0cf8 to aad3090 Compare September 27, 2021 09:45
}
}
EOL
cat ./cmd/docker/versioninfo.json
Copy link
Member

Choose a reason for hiding this comment

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

This is just for debugging, correct? (wondering if it should do this by default, or left to the consumer of this script)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah just for debugging and also audit in CI.

@crazy-max crazy-max force-pushed the goversioninfo branch 2 times, most recently from 2ac67d5 to a4ae4f1 Compare September 27, 2021 10:19
@crazy-max crazy-max requested a review from thaJeztah September 27, 2021 11:49
@crazy-max crazy-max force-pushed the goversioninfo branch 4 times, most recently from b0a73c7 to 341292d Compare October 4, 2021 13:48
@crazy-max
Copy link
Member Author

PTAL @thaJeztah @silvin-lubecki @mat007

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

/cc @cpuguy83 (in case you're using this)

@thaJeztah thaJeztah added this to the 21.xx milestone Oct 7, 2021
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

We discussed this change in the maintainers meeting, and everyone agreed that this was a nicer solution than what we had before, so I'll bring this in once CI finishes 👍

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.

4 participants