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

Print path to compiler in output #1328

Merged
merged 12 commits into from
Mar 4, 2024

Conversation

data-queue
Copy link
Contributor

For each call to detect_compiler, print the detected compiler path. Alternative to #1320

@Thomas1664
Copy link
Contributor

What's the underlying goal of these PRs?

@data-queue
Copy link
Contributor Author

From PR #1320:

On Windows, if you have a side by side installation of Visual Studio, you may run into the issue where the compiler that vcpkg uses is different than the one set by the developer command prompt.

After the team talked about it, we felt the best path forward was to print the path to the detected compiler.

@Thomas1664
Copy link
Contributor

Thomas1664 commented Jan 25, 2024

Do I understand it correctly that you just want to make the user aware that the problem exists instead of fixing it by prioritising the toolset inside the developer command prompt higher than the latest Visual Studio installation?

Furthermore, compiler detection doesn't work the same way on all platforms, e.g. I have GCC 11 and GCC 12 but vcpkg selects GCC 11 although I built my cmake project with GCC 12 and vcpkg is invoked through cmake. I think you should detect this kind of compiler mismatch on all platforms.

@data-queue
Copy link
Contributor Author

Yes, the goal for this PR is to make the user aware of the problem (this was an ask by an internal customer), and leave it up to the user to select the correct compiler in the toolchain or triplet.

That's a good point, and vcpkg should probably detect the compiler mismatch in your example.

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I think this needs at least one e2e test verifying that this output is ever printed. I don't see it:
image

@BillyONeal
Copy link
Member

I see now you did have corresponding detect_compiler changes. I think you should print the version if we don't have the path for previous detect_compilers rather than printing blank.

@data-queue
Copy link
Contributor Author

Instead, I decided to not print a message if detect_compiler doesn't have the path

Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

I think this still needs an e2e test. (Not blocking over my tiny nitpick :) )

static constexpr StringLiteral s_path_marker = "#COMPILER_CXX_PATH#";
if (Strings::starts_with(s, s_path_marker))
{
compiler_info.path = s.substr(s_path_marker.size()).to_string();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
compiler_info.path = s.substr(s_path_marker.size()).to_string();
const auto compiler_cxx_path = s.substr(s_path_marker.size());
compiler_info.path.assign(compiler_cxx_path.data(), compiler_cxx_path.size());

This extreme nitpick avoids an extra temporary string.

@BillyONeal
Copy link
Member

To fix the test failures you can update https://github.com/microsoft/vcpkg-tool/blob/main/vcpkg-init/vcpkg-scripts-sha.txt to include your changes there

@BillyONeal
Copy link
Member

The linux failure is fixed by microsoft/vcpkg#37129

@data-queue data-queue merged commit 13fd7ab into microsoft:main Mar 4, 2024
5 checks passed
@data-queue data-queue deleted the print-compiler-path branch March 4, 2024 22:59
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.

3 participants