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

Fixed issue with output from CMake 3.21.0 #107

Merged
merged 2 commits into from
Jul 14, 2021

Conversation

MatthewPowley
Copy link
Contributor

When calling CMake to obtain triplet information a regression change from CMake 3.20.5 to 3.21.0-rc1
causes the tool to exit due to an exception.

This is due to the response containing a mixture of '\n' and '\r\n' line endings.

After splitting the output on line feeds ('\n') each line is then trimmed of whitespace to remove
any carriage returns ('\r') or trailing whitespace. This could be fixed in CMake but this
change makes it robust.

This commit in CMake caused the fault: Kitware/CMake@0a0a0f8
It is unlikely to change as they have added a new helper file for writing the terminal output.

When calling CMake to obtain triplet information a regression change from CMake 3.20.5 to 3.21.0-rc1
causes the tool to exit due to an exception.

This is due to the response containing a mixture of 'n' and  '\r\n' line endings.

After splitting the output on line feeds ('\n') each line is then trimmed of whitespace to remove
any carriage returns ('\r') or trailing whitespace. This could be fixed in CMake but this
change makes it robust.

This commit in CMake caused the fault: Kitware/CMake@0a0a0f8
It is unlikely to change as they have added a new helper file for writing the terminal output
@MatthewPowley
Copy link
Contributor Author

MatthewPowley commented Jul 7, 2021

Fixes these issues reported in Vcpkg.
microsoft/vcpkg#18718
microsoft/vcpkg#18804
microsoft/vcpkg#18806
microsoft/vcpkg#18636

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

We call CMake from several different locations; we need to audit each of these for potential \r\n mishandling.

src/vcpkg/cmakevars.cpp Outdated Show resolved Hide resolved
@MatthewPowley
Copy link
Contributor Author

I have had a look through the places where CMake is called and apart from this instance they either use 'cmd_execute_and_stream_lines', which your pull request #108 handles, or they are written directly to the console.

I agree the above is aggressive to remove trailing '\r' characters. (I made the change quickly as I was proving out a large system build using Visual Studio 2022 and I came across this blocking issue).

The code could be consistent with other CMake calls in the system by also using 'cmd_execute_and_stream_lines'.

        std::vector<std::string> lines;
        auto const exit_code = cmd_execute_and_stream_lines(
            cmd_launch_cmake, [&](StringView sv) { lines.emplace_back(sv.begin(), sv.end()); });

        Checks::check_exit(VCPKG_LINE_INFO, exit_code == 0, Strings::join("\n", lines));

        const auto end = lines.cend();

        auto port_start = std::find(lines.cbegin(), end, PORT_START_GUID);
        ...

The only 'nasty' bit I don't like is having to join the lines again in case of an error.

What is your preference?

@ras0219-msft
Copy link
Contributor

ras0219-msft commented Jul 8, 2021

Yeah I like your suggested unification 👍 . The idea of the lines join is fine, though unfortunately the code as written will execute the join on all cases (including non-error). It'd be better to do:

if (exit_code != 0)
{
    Checks::exit_with_message(VCPKG_LINE_INFO, Strings::join("\n", lines));
}

Assuming Pull Request microsoft#108 (Add \r\n support to System::cmd_execute_and_stream_lines) has been
merged. Calling cmd_execute_and_stream_lines will be the safer and more efficient solution
@strega-nil-ms
Copy link
Contributor

Fixed by #107, I believe?

@strega-nil-ms
Copy link
Contributor

Oh, cool, thanks @MatthewPowley :)

@strega-nil-ms strega-nil-ms merged commit 8b1c04b into microsoft:main Jul 14, 2021
@cenit
Copy link
Contributor

cenit commented Jul 15, 2021

a release with this PR is very urgent to be deployed in vcpkg because cmake 3.21 is now released and many CI tests using :latest cmake version are now failing :)

@bradking
Copy link

@MatthewPowley thanks for trying the 3.21 release candidates and tracking this down.

This is due to the response containing a mixture of '\n' and '\r\n' line endings.

I think it is good to tolerate both newline styles in vcpkg.

It is unlikely to change as they have added a new helper file for writing the terminal output.

The newline change was not intentional. I've opened CMake Issue 22444 to discuss this on the CMake side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants