-
Notifications
You must be signed in to change notification settings - Fork 192
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
Use MSVC for the Windows toolchain instead of MinGW #456
Use MSVC for the Windows toolchain instead of MinGW #456
Conversation
# msys/mingw might bring. | ||
# | ||
# As of 2024-07-22 this sha is the "v1" tag. | ||
- uses: ilammy/msvc-dev-cmd@0b201ec74fa43914dc39ae48a89fd1d8cb592756 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was debating between the need for this action versus the risk: it looks like ~17k other projects depend on this, it has more than a few contributors, and the action is relatively simple — if it solves the problem then let's go for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wrestling with the same. I spend 30 min or so googling around to see if there were other common solutions to this but everything pointed to a github action one way or another so I opted to do that with a specific sha to at least be insulated against changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
personally, I would fork any dep like this (in case the repo gets deleted, repurposed, ...)
Explicitly use MSVC to avoid the runtime dependencies that the default toolchain CMake is using is bringing in. Closes WebAssembly#454
Co-authored-by: Andrew Brown <andrew.brown@intel.com>
Looks like MSVC defaults to "Debug" instead of an empty string to so the default logic which works for other compilers isn't kicking in here.
dee31bd
to
d9a25b5
Compare
Got a successful CI run, but I'm going to rerun CI to confirm that caching makes it so the next run isn't 2 hours. Afterwards I'll download this on a windows machine and test that things work as expected. |
I was able to compile dotnet/mono using the artifact from this PR on my local windows |
The "cached build" took 2+ hours so I'm going to try to dig in to why ccache isn't working. |
Hm on a new commit it worked with a ~20m build time, similar to today. Given that I think this is ok, so I'm going to merge. |
Explicitly use MSVC to avoid the runtime dependencies that the default toolchain CMake is using is bringing in.
Closes #454