Skip to content
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

Fix build failures with latest MSVC (main) #2598

Merged
merged 7 commits into from
Jun 15, 2023

Conversation

saxena-anurag
Copy link
Contributor

@saxena-anurag saxena-anurag commented Jun 14, 2023

Description

Looks like the MSVC version has again changed from 14.35.32215 to 14.36.32532 and broke the build for the main branch. This PR fixes the build failures and contains the following changes:

  1. Use vcvars64 and set the environment variables for VCToolsInstallDir and VCToolsVersion, so that we dont hardcode path in the yml.
  2. Fixes for new analysis failures flagged by new MSVC version.

Testing

Existing CICD

Documentation

NA

@saxena-anurag saxena-anurag changed the title [DRAFT]: DO NOT REVIEW Update MSVC path Jun 14, 2023
@saxena-anurag saxena-anurag marked this pull request as ready for review June 14, 2023 22:17
gtrevi
gtrevi previously approved these changes Jun 14, 2023
@@ -176,7 +176,7 @@ jobs:
- name: Copy LLVM libs for Fuzzing & Address Sanitizing
if: steps.skip_check.outputs.should_skip != 'true'
working-directory: ./${{env.BUILD_PLATFORM}}/${{env.BUILD_CONFIGURATION}}
run: copy "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.35.32215\bin\Hostx64\x64\clang*" .
run: copy "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\bin\Hostx64\x64\clang*" .
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you use the VCToolsInstallDir environment variable to automatically get the right path and not have to change it when github updates the msvc version?

Copy link
Collaborator

@gtrevi gtrevi Jun 14, 2023

Choose a reason for hiding this comment

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

@dthaler
@saxena-anurag

Yes, it can be done, but still the VS version and product type would have to be hard-coded (i.e. "2022", and "Enterprise"/"Community"/etc.), if the current console does not have the VC environment variables:

# Generate the Visual Studio environment variables (if not already done)
> call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat"

# Parse the output string (or just use the variable)
> echo %VCToolsInstallDir%
C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Tools\MSVC\14.36.32532\

Something like

call "C:\Program Files\Microsoft Visual Studio\2022\Enterprise\VC\Auxiliary\Build\vcvars64.bat"
copy %VCToolsInstallDir%bin\Hostx64\x64\clang*

Copy link
Collaborator

Choose a reason for hiding this comment

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

github preinstalls Visual Studio so that variable should always be set I think

Copy link
Collaborator

Choose a reason for hiding this comment

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

The VS environment variables are not installed as System defaults, i.e. that's why VS offers a "VS Command Prompt", which basically just runs that .bat script.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a quick test and %VCToolsInstallDir% is not available by default in the runner.

Copy link
Collaborator

@dthaler dthaler Jun 14, 2023

Choose a reason for hiding this comment

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

Maybe use the VSVARS32 script to get the variable set to save to github environment variables then for future steps.

Copy link
Contributor Author

@saxena-anurag saxena-anurag Jun 15, 2023

Choose a reason for hiding this comment

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

Updated PR to use environment variable. Also updated the cache hash to use the VS tool version.

@codecov
Copy link

codecov bot commented Jun 14, 2023

Codecov Report

Merging #2598 (e833fac) into main (3e77670) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2598      +/-   ##
==========================================
+ Coverage   83.91%   83.96%   +0.05%     
==========================================
  Files         157      157              
  Lines       29173    29177       +4     
==========================================
+ Hits        24480    24499      +19     
+ Misses       4693     4678      -15     
Impacted Files Coverage Δ
...ution_context/unit/execution_context_unit_test.cpp 98.88% <ø> (ø)
tests/end_to_end/test_helper.cpp 86.82% <ø> (ø)
tests/libs/util/socket_helper.cpp 49.52% <100.00%> (+0.15%) ⬆️

... and 11 files with indirect coverage changes

@saxena-anurag saxena-anurag changed the title Update MSVC path Update MSVC path (main branch) Jun 15, 2023
@saxena-anurag saxena-anurag changed the title Update MSVC path (main branch) Update MSVC path (main) Jun 15, 2023
@saxena-anurag saxena-anurag changed the title Update MSVC path (main) Fix build failures with latest MSVC (main) Jun 15, 2023
@saxena-anurag saxena-anurag added this pull request to the merge queue Jun 15, 2023
Merged via the queue into microsoft:main with commit b5d0dfa Jun 15, 2023
@saxena-anurag saxena-anurag deleted the user/anusa/test_pr branch April 24, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants