Skip to content

cmd: fix winresources and move them out cli package#50269

Merged
thaJeztah merged 3 commits intomoby:masterfrom
crazy-max:fix-winres
Jun 26, 2025
Merged

cmd: fix winresources and move them out cli package#50269
thaJeztah merged 3 commits intomoby:masterfrom
crazy-max:fix-winres

Conversation

@crazy-max
Copy link
Member

@crazy-max crazy-max commented Jun 26, 2025 •
edited by thaJeztah
Loading

- What I did

Fixes regression from #50010 where win resources are not included in the binary anymore:

$ docker buildx bake --set *.platform=windows/amd64 https://github.com/moby/moby.git#ee49437e38bb457a7724922c21610aaa4d9c4282 binary
...
$ wrestool -l ./bundles/binary/dockerd.exe
wrestool: ./bundles/binary/dockerd.exe: file contains no resources

Previously:

$ docker buildx bake --set *.platform=windows/amd64 https://github.com/moby/moby.git#v28.3.0 binary
...
$ wrestool -l ./bundles/binary/dockerd.exe
--type=3 --name=1 --language=0 [type=icon offset=0x46e7268 size=2636]
--type=3 --name=2 --language=0 [type=icon offset=0x46e7cb8 size=67624]
--type=3 --name=3 --language=0 [type=icon offset=0x46f84e0 size=16936]
--type=3 --name=4 --language=0 [type=icon offset=0x46fc708 size=9640]
--type=3 --name=5 --language=0 [type=icon offset=0x46fecb0 size=4264]
--type=3 --name=6 --language=0 [type=icon offset=0x46ffd58 size=1128]
--type=11 --name=1 --language=1033 [type=messagelist offset=0x47001c0 size=256]
--type=14 --name=1 --language=1033 [type=group_icon offset=0x47002c0 size=90]
--type=16 --name=1 --language=0 [type=version offset=0x4700320 size=588]
--type=24 --name=1 --language=1033 [offset=0x4700570 size=1186]

This also:

  • Moves winresources package directly in ./cmd/dockerd as winresources are not uses anymore by docker-proxy. Effectively removing cli package.
  • Use Dockerfile to generate event messages with windmc
  • Verify that the built binary contains windows resources

- How to verify it

$ docker buildx bake --set *.platform=windows/amd64 https://github.com/moby/moby.git#refs/pull/50268/merge binary
...
$ wrestool -l ./bundles/binary/dockerd.exe
--type=3 --name=1 --language=0 [type=icon offset=0x4705268 size=2636]
--type=3 --name=2 --language=0 [type=icon offset=0x4705cb8 size=67624]
--type=3 --name=3 --language=0 [type=icon offset=0x47164e0 size=16936]
--type=3 --name=4 --language=0 [type=icon offset=0x471a708 size=9640]
--type=3 --name=5 --language=0 [type=icon offset=0x471ccb0 size=4264]
--type=3 --name=6 --language=0 [type=icon offset=0x471dd58 size=1128]
--type=11 --name=1 --language=1033 [type=messagelist offset=0x471e1c0 size=256]
--type=14 --name=1 --language=1033 [type=group_icon offset=0x471e2c0 size=90]
--type=16 --name=1 --language=0 [type=version offset=0x471e320 size=588]
--type=24 --name=1 --language=1033 [offset=0x471e570 size=1186]

@crazy-max
cmd: fix winresources and move them out cli package
0ea20c9 
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@crazy-max crazy-max force-pushed the fix-winres branch 3 times, most recently from a721b0e to b01698b Compare June 26, 2025 14:16
@crazy-max crazy-max marked this pull request as ready for review June 26, 2025 14:17
@crazy-max crazy-max requested a review from tianon as a code owner June 26, 2025 14:17
crazy-max
crazy-max commented Jun 26, 2025
View reviewed changes
hack/make/.binary
Comment on lines +100 to +110
# Verify that the built binary contains windows resources
if [ "$(go env GOOS)" = "windows" ]; then
if command -v wrestool > /dev/null 2>&1; then
if ! wrestool -l "$DEST/$BINARY_FULLNAME" | grep -q -- '--type='; then
echo "No resources found in $DEST/$BINARY_FULLNAME"
exit 1
fi
else
echo "WARNING: wrestool not found, skipping resources check"
fi
fi
Copy link
Member Author

@crazy-max crazy-max Jun 26, 2025

Choose a reason for hiding this comment

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

We should do the same in make.ps1 as follow-up.

dmcgowan
dmcgowan approved these changes Jun 26, 2025
View reviewed changes
Copy link
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

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

Thanks for the quick fix. Would it be easy in the future to ensure that CI checks this?

@crazy-max
Copy link
Member Author

crazy-max commented Jun 26, 2025

Would it be easy in the future to ensure that CI checks this?

This is done with wrestool tool, see last commit

@dmcgowan dmcgowan added this to the 29.0.0 milestone Jun 26, 2025
@thaJeztah
Copy link
Member

thaJeztah commented Jun 26, 2025

Thanks! I added fixes/closes links to my old PR and the related ticket ❤️

thaJeztah
thaJeztah reviewed Jun 26, 2025
View reviewed changes
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.

overall LGTM - left a question about the # syntax used 😅

cmd/dockerd/winresources/event_messages.h
Comment on lines +1 to +2
/* Do not edit this file manually.
This file is autogenerated by windmc. */
Copy link
Member

@thaJeztah thaJeztah Jun 26, 2025

Choose a reason for hiding this comment

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

I guess this comment is out of our control; would be nice if it could match the Go standardised "generated" format 😓

// Code generated by XYZ. DO NOT EDIT.

Copy link
Member Author

@crazy-max crazy-max Jun 26, 2025

Choose a reason for hiding this comment

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

Yes this is generated by windmc

hack/make/.go-autogen.ps1
Comment on lines 72 to 74
$mkwinresContents = '{
"RT_GROUP_ICON": {
"#1": {
Copy link
Member

@thaJeztah thaJeztah Jun 26, 2025

Choose a reason for hiding this comment

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

Still curious (not for this PR) if we could change all of this to be a go generate ./... without special scripts (at least, I could see that writing a JSON should be possible); devils is probably in the details (version strings?)

Copy link
Member Author

@crazy-max crazy-max Jun 26, 2025

Choose a reason for hiding this comment

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

I think that would work yes, can take a look as follow-up.

@thaJeztah thaJeztah mentioned this pull request Sep 20, 2024
Closed
5 tasks
crazy-max and others added 2 commits June 26, 2025 17:03
@crazy-max
cmd: use dockerfile to generate win event messages
44623fb 
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
@crazy-max
hack: check windows resources are set in the binary
e7289e7 
Signed-off-by: CrazyMax <1951866+crazy-max@users.noreply.github.com>
thaJeztah
thaJeztah approved these changes Jun 26, 2025
View reviewed changes
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 thaJeztah merged commit a6be38a into moby:master Jun 26, 2025
181 of 182 checks passed
@crazy-max crazy-max deleted the fix-winres branch June 26, 2025 19:01
@crazy-max crazy-max self-assigned this Jul 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@thaJeztah thaJeztah thaJeztah approved these changes

@vvoland vvoland Awaiting requested review from vvoland

@tianon tianon Awaiting requested review from tianon tianon is a code owner

Assignees

@crazy-max crazy-max

Labels

area/packaging kind/bugfix PR's that fix bugs platform/windows status/2-code-review

Projects

None yet

Milestone

29.0.0

Development

Successfully merging this pull request may close these issues.

[epic] remove the obsolete "cli" package

4 participants

@crazy-max @thaJeztah @dmcgowan @vvoland