Skip to content

Conversation

@zsy056
Copy link
Contributor

@zsy056 zsy056 commented Nov 13, 2025

Refresh msvc2017 Dockerfile and add windows workflow

  • Did you create an Apache Jira ticket? (Request account here, not required for trivial changes)
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
  • If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

@CJCombrink
Copy link
Contributor

FYI: I am also looking at a Windows Action but taking a bit of a different approach: CJCombrink#7

@zsy056
Copy link
Contributor Author

zsy056 commented Nov 13, 2025

FYI: I am also looking at a Windows Action but taking a bit of a different approach: CJCombrink#7

Cool! Your workflow runs much faster :D. The part I like about using docker is that it publishes a thrift-build image that can help folks get started with a ready-to-use environment. Maybe I can rename the workflow to something like msvc.yml.

@zsy056 zsy056 force-pushed the windows-docker branch 2 times, most recently from 3e3c644 to d22c693 Compare November 13, 2025 18:24
@Jens-G
Copy link
Member

Jens-G commented Nov 13, 2025

does that include net10?

@Jens-G Jens-G added the build and general CI cmake, automake and build system changes label Nov 13, 2025
@zsy056
Copy link
Contributor Author

zsy056 commented Nov 13, 2025

does that include net10?

I don't think so, I can try to add the support.

@Jens-G
Copy link
Member

Jens-G commented Nov 14, 2025

does that include net10?

I don't think so, I can try to add the support.

I'm not so sure if this is really needed, sorry. Not sure what I had in mind when I asked that question.

@zsy056
Copy link
Contributor Author

zsy056 commented Nov 14, 2025

workflow failed as expected, I think the failed python test is being fixed in #3231

Write-Error: Tests failed - check LastTest.log artifact for details
 Executing individual test scripts with various generated code directories
 Directories to be tested: gen-py-default, gen-py-slots, gen-py-oldstyle, gen-py-no_utf8strings, gen-py-dynamic, gen-py-dynamicslots, gen-py-enum, gen-py-type_hints
 Scripts to be tested: FastbinaryTest.py, TestFrozen.py, TestRenderedDoubleConstants.py, TSimpleJSONProtocolTest.py, SerializationTest.py, TestEof.py, TestSyntax.py, TestSocket.py, TestTypes.py
----------------
Traceback (most recent call last):
  File "C:\thrift\test\py\RunClientServer.py", line 334, in <module>
    sys.exit(main())
             ~~~~^^
  File "C:\thrift\test\py\RunClientServer.py", line 313, in main
    runScriptTest(options.libdir, options.gen_base, genpydir, script)
    ~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\thrift\test\py\RunClientServer.py", line 93, in runScriptTest
    env = setup_pypath(libdir, os.path.join(genbase, genpydir))
  File "C:\thrift\test\py\RunClientServer.py", line 86, in setup_pypath
    env['PYTHONPATH'] = os.pathsep.join(dirs)
                        ~~~~~~~~~~~~~~~^^^^^^
TypeError: sequence item 0: expected str instance, NoneType found
<end of output>
Test time =   0.20 sec
----------------------------------------------------------
Test Failed.

@CJCombrink
Copy link
Contributor

The part I like about using docker is that it publishes a thrift-build image that can help folks get started with a ready-to-use environment.

Sure it does have some benefit and it builds on top of what is already existing. Although I am not sure who is brave enough to do local docker based development on Windows :P

Just regarding the approach: I opted to use conan as a package manager instead of building dependencies manually. I think conan is more inclined to keep dependencies building and up to data compared to trying to maintain basically the same logic here. Conan also provides some pre-build packages that can speed up the build.

ADD https://boost.teeks99.com/bin/1.69.0/boost_1_69_0-msvc-14.1-64.exe C:\TEMP\boost.exe
RUN C:\TEMP\boost.exe /DIR="C:\Libraries\boost_1_69_0" /SILENT && `
DEL C:\TEMP\boost.exe
ADD https://boost.teeks99.com/bin/1.88.0/boost_1_88_0-msvc-14.1-64.exe C:\TEMP\boost.exe
Copy link
Contributor

Choose a reason for hiding this comment

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

I can only recommend adding --checksum checking either to the ADD or do it manually to add some layer of protection and verification.

@zsy056
Copy link
Contributor Author

zsy056 commented Nov 17, 2025

The part I like about using docker is that it publishes a thrift-build image that can help folks get started with a ready-to-use environment.

Sure it does have some benefit and it builds on top of what is already existing. Although I am not sure who is brave enough to do local docker based development on Windows :P

Just regarding the approach: I opted to use conan as a package manager instead of building dependencies manually. I think conan is more inclined to keep dependencies building and up to data compared to trying to maintain basically the same logic here. Conan also provides some pre-build packages that can speed up the build.

I totally agree using Conan or vcpkg would be much better. I can switch the build script to use Conan after your change got merged.

Just curious, what is wrong about local docker based development on Windows? 🤣

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

Labels

build and general CI cmake, automake and build system changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants