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

Introduce --x-cmake-debug and --x-cmake-configure-debug #1173

Merged
merged 4 commits into from
Sep 4, 2023

Conversation

Neumann-A
Copy link
Contributor

@Neumann-A Neumann-A commented Aug 28, 2023

Debug with e.g.

./vcpkg.exe install zlib --x-cmake-debug \\.\pipe\tespipe;zlib
for portfile debugging
./vcpkg.exe install zlib --x-cmake-configure-debug \\.\pipe\tespipe;zlib
for debugging cmake configure of the port

(In Git bash \ needs to be escaped to \\)

or more general
./vcpkg.exe install zlib --x-cmake(-configure)-debug <valid_pipe_name>(;semicolon_seperated_list_of_ports_to_debug)
for debugging cmake configure of the port

Requires external cmake debugger to attach to the opened pipe to continue execution (which is why I did not write tests for it since it would at least require two seperate process started at the right time to test this.).

Docs PR microsoft/vcpkg-docs#143

BillyONeal and others added 3 commits August 28, 2023 17:29
* Deduplicate the logic of "no entries means no filter, one or more entries means port filter"
* Add unit test
Comment on lines +164 to +183
TEST_CASE ("CMake debugger flags", "[arguments]")
{
std::vector<std::string> t = {"--x-cmake-debug",
"\\\\.\\pipe\\tespipe;zlib;bar;baz",
"--x-cmake-configure-debug",
"\\\\.\\pipe\\configure-pipe"};
auto v = VcpkgCmdArguments::create_from_arg_sequence(t.data(), t.data() + t.size());
auto& cmake_debug = v.cmake_debug.value_or_exit(VCPKG_LINE_INFO);
REQUIRE(cmake_debug.value == "\\\\.\\pipe\\tespipe");
REQUIRE(!cmake_debug.is_port_affected("7zip"));
REQUIRE(cmake_debug.is_port_affected("zlib"));
REQUIRE(cmake_debug.is_port_affected("bar"));
REQUIRE(cmake_debug.is_port_affected("baz"));
REQUIRE(!cmake_debug.is_port_affected("bazz"));

auto& cmake_configure_debug = v.cmake_configure_debug.value_or_exit(VCPKG_LINE_INFO);
REQUIRE(cmake_configure_debug.value == "\\\\.\\pipe\\configure-pipe");
REQUIRE(cmake_configure_debug.is_port_affected("7zip"));
REQUIRE(cmake_configure_debug.is_port_affected("zlib"));
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only tests PortApplicableSetting and not the actual cmake invocation.
I am more concerned about cmake <something> -P <script> being a valid cmake call.

Copy link
Member

Choose a reason for hiding this comment

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

That would require setting up, as you said, the whole separate processes business. I don't have good ideas on testing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then testing by direct usage? I mean it is an --x- option any way.

@@ -290,6 +316,18 @@ namespace vcpkg
StabilityTag::Experimental,
args.asset_sources_template_arg,
msg::format(msgAssetSourcesArg));
{
std::string raw_cmake_debug;
if (args.parser.parse_option(CMAKE_DEBUGGING_ARG, StabilityTag::Experimental, raw_cmake_debug))
Copy link
Contributor Author

@Neumann-A Neumann-A Aug 30, 2023

Choose a reason for hiding this comment

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

I wonder if parse_option couldn't be taught to do this automatically if fulfills the concept of being StringConstructable.

Copy link
Member

Choose a reason for hiding this comment

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

It could but then parse_option would have to be a template... if it were used in more than 1 place I think that'd be worth it but at least yet I don't think so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@BillyONeal
Copy link
Member

I think this is ready to land as soon as we have signoff on the corresponding docs PR.

@Neumann-A
Copy link
Contributor Author

@BillyONeal Docs PR microsoft/vcpkg-docs#143

(Hopefully it doesn't get ghosted like the other docs PRs. )

@BillyONeal BillyONeal merged commit efcf58d into microsoft:main Sep 4, 2023
5 checks passed
@BillyONeal
Copy link
Member

Thanks for your contribution!

@Neumann-A Neumann-A deleted the cmake_debugging branch September 4, 2023 22:44
AugP added a commit to microsoft/vcpkg-docs that referenced this pull request Sep 5, 2023
* Add docs for --x-cmake-debug and --x-cmake-configure-debug

See microsoft/vcpkg-tool#1173

* Add .mds.

* add additional info

* Update vcpkg/commands/common-options.md

Co-authored-by: Victor Romero <[email protected]>

* Minor text changes, added a URL to classic mode docs

---------

Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: Victor Romero <[email protected]>
Co-authored-by: Augustin Popa <[email protected]>
vicroms added a commit to microsoft/vcpkg-docs that referenced this pull request Sep 25, 2023
* Document "Reference/Commands/vcpkg contact" (#129)

* add docs for contact

* Update vcpkg/commands/contact.md

---------

Co-authored-by: Victor Romero <[email protected]>

* Add docs for --x-cmake-debug and --x-cmake-configure-debug (#142)

* Add docs for --x-cmake-debug and --x-cmake-configure-debug

See microsoft/vcpkg-tool#1173

* Add .mds.

* add additional info

* Update vcpkg/commands/common-options.md

Co-authored-by: Victor Romero <[email protected]>

* Minor text changes, added a URL to classic mode docs

---------

Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: Victor Romero <[email protected]>
Co-authored-by: Augustin Popa <[email protected]>

---------

Co-authored-by: Javier Matos Denizac <[email protected]>
Co-authored-by: Billy Robert O'Neal III <[email protected]>
Co-authored-by: Alexander Neumann <[email protected]>
Co-authored-by: Augustin Popa <[email protected]>
@benmcmorran
Copy link
Member

@Neumann-A FYI we just published a blog post on using these flags with the CMake Tools VS Code extension. Thanks again for your contribution!

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