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

Add default compiler search paths for msys2 default installation paths, fix mingw bugs #3056

Merged
merged 4 commits into from
Mar 18, 2023

Conversation

philippewarren
Copy link
Contributor

@philippewarren philippewarren commented Mar 14, 2023

This change addresses items #2880, #1064, #2447 and maybe #460 and parts of #2640, #1669

This changes visible behavior

The following changes are proposed:

  • MSYS2 default installation folders (mingw64, mingw32, ucrt64, clang64, clang32 and clangarm64) are now scanned by default
  • MinGW path is now correctly added when using a MinGW kit (was not added correctly)
  • MinGW path is now prepended instead of appended (to allow using a MinGW environment even if there is another one in the standard PATH)
  • The configuration option cmake.mingwSearchDirs is now cmake.additionalCompilerSearchDirs
  • The value of cmake.mingwSearchDirs/cmake.additionalCompilerSearchDirs is now correctly used when scanning, no matter how the scan is performed (was only used when the scan was performed from the command CMake: Scan for kits)
  • The compiler name now includes the name of the parent directory of the parent directory of the compiler (i.e. for a compiler C:\msys64\mingw64\bin\gcc.exe, the name will include (mingw64) to prevent removal of different compilers with the exact same name, but different MinGW environments.

The purpose of this change

Fixes bugs with MinGW environments.
Allows to more easily use MSYS2-installed MinGW environments.
Allows to use alternative MSYS2 MinGW environments (clang64, ucrt64, etc.)

Other Notes/Information

Feel free to suggest any other improvements ideas concerning MSYS2 and/or MinGW support.

@philippewarren
Copy link
Contributor Author

@microsoft-github-policy-service agree

@benmcmorran
Copy link
Member

Thanks for the contribution! I just kicked off the test runs which found a few linter errors, and I should get a chance to fully review the changes within the next few days. Note that changes in our security policy mean that maintainers now have to manually queue each test run for external contributions, but I'll keep an eye on this PR.

@benmcmorran benmcmorran added this to the On Deck milestone Mar 15, 2023
Copy link
Member

@benmcmorran benmcmorran left a comment

Choose a reason for hiding this comment

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

In addition to my other comments, please update the 1.14 section in CHANGELOG.md with a brief description of your changes and optionally give yourself credit (see the other examples in the file). Otherwise this is looking good. Thanks!

src/kit.ts Outdated
@@ -389,6 +407,11 @@ export async function kitIfCompiler(bin: string, pr?: ProgressReporter): Promise
// mentions msvc.
return null;
}
if (version.target && version.target.triple.includes('msvc') && clang_cl_res && isMingw(bin)) {
// Skip clang-cl.exe from mingw, as it won't work (correct access to MSCV environment is not granted).
Copy link
Member

Choose a reason for hiding this comment

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

Typo: MSCV -> MSVC

get mingwSearchDirs(): string[] {
return this.configData.mingwSearchDirs;
get additionalCompilerSearchDirs(): string[] {
return this.configData.additionalCompilerSearchDirs;
Copy link
Member

Choose a reason for hiding this comment

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

For backwards compatibility, continue returning this.configData.mingwSearchDirs here if this.configData.additionalCompilerSearchDirs isn't set (and add mingwSearchDirs back to ExtensionConfigurationSettings). We can consider the old setting deprecated and not document it, but I'd like to avoid breaking existing configurations if possible.

@benmcmorran benmcmorran enabled auto-merge (squash) March 18, 2023 07:08
@benmcmorran benmcmorran merged commit 39c7266 into microsoft:main Mar 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators May 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants