Repository navigation
THRIFT-6315: Correct the Windows Docker documentation - #3917
Merged
Merged
Conversation
Client: build The MSVC container directory was renamed from build/docker/msvc2017 to build/docker/msvc, and the paths naming it were not updated. doc/ReleaseManagement.md had a broken relative link, and the README in that directory told the reader to run commands against paths that do not exist, six times. Three other things in that README had also gone stale. It said to pull thrift/thrift-build:msvc2017 from Docker Hub. That repository carries the Linux images only - its tags are ubuntu-*, debian-* and centos - so the pull could not have worked. The MSVC workflow builds the image itself and caches it in GHCR under a tag derived from the hash of the Dockerfile and the scripts it copies, which is now what the README describes. The build context is build\, not the Dockerfile's own directory, which was not said anywhere and is easy to get wrong. It said the compiler is statically linked and depends only on KERNEL32.DLL. That stopped being true when the project moved away from a statically linked compiler; THRIFT-6320 corrected the same claim in ReleaseManagement.md. The README now says what is actually expected - the Visual C++ redistributable, and nothing else beyond Windows system DLLs - and points at check-compiler-imports.ps1 and the compiler-windows job, which check it on every push. It said the image is "just under 30GB" and that things should improve "once Microsoft supports build tools in nano". THRIFT-5952 and THRIFT-6237 have both worked on the size since, so the number is not one to quote; it says the image is large and leaves it there. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mergeable
Bot
added
the
build and general CI
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Doc-only. Noticed while adding the Windows compiler CI job; filed then rather than folded in.
The MSVC container directory was renamed
build/docker/msvc2017→build/docker/msvcand the paths naming it were not updated:doc/ReleaseManagement.mdhad a broken relative link, and the README in that directory told the reader to run commands against paths that do not exist — six times.While in there, three other things had gone stale.
The pull instruction could not have worked
It said to
docker pull thrift/thrift-build:msvc2017. That Docker Hub repository carries the Linux images only — I checked the tag list:No
msvc2017, no Windows tag at all.msvc.ymlbuilds the image itself and caches it in GHCR under a tag derived from the hash of the Dockerfile and the scripts it copies, so a run only rebuilds when one of those changes. That is what the README now describes.It also now says the build context is
build\, not the Dockerfile's own directory — necessary because the image copies frombuild\appveyor\, easy to get wrong, and previously stated nowhere.It still claimed a statically linked compiler
That stopped being true when the project moved away from a statically linked compiler. #3901 (THRIFT-6320) corrected exactly this claim in
ReleaseManagement.mdand left this copy behind.It now states what is actually expected — the Visual C++ redistributable, and nothing else beyond Windows system DLLs — and points at
check-compiler-imports.ps1and thecompiler-windowsjob, which assert it on every push.Size and nano
THRIFT-5952 and THRIFT-6237 have both worked on the image size since, so that number is not one to quote. It now just says the image is large. The nano remark went with it; the base image is
mcr.microsoft.com/dotnet/sdk:10.0-windowsservercore-ltsc2025.Checked
No
msvc2017remains anywhere in the tree, and every relative link in the rewritten README resolves from the README's own directory (not just from the repo root, which is the easy way to get this wrong):JIRA: THRIFT-6315
🤖 Generated with Claude Code