Reduce invalid output of cmake usage heuristics #849

dg0yt · 2023-01-08T17:39:53Z

For microsoft/vcpkg#20190.

Any deviation from the usual generated form calls for manual action.

vcpkg wants config in 'share/${PORT}/' which maps to CMake's '<prefix>/(lib/<arch>|lib|share)/<name>*/' pattern, i.e. the beginning of the directory name must match the package name, case-insensitively.

Correlate package names and targets per 'share/${PORT}'. Ignore nested dirs.

If both non-namespaced and namespaced targets exist, assume that the non-namespaced target is newer and preferred. Example: xerces_xerces-c -> XercesC::XercesC

dg0yt · 2023-01-08T20:53:39Z

./vcpkg install grpc :

 grpc provides CMake targets:
 
     # this is heuristically generated, and may not be correct
     find_package(gRPC CONFIG REQUIRED)
     # note: 7 additional targets are not displayed.
     target_link_libraries(main PRIVATE gRPC::gpr gRPC::grpc gRPC::grpc++ gRPC::grpc++_alts)
 
-    find_package(modules CONFIG REQUIRED)
-    target_link_libraries(main PRIVATE re2::re2 c-ares::cares)
-

dg0yt · 2023-01-08T20:56:26Z

xerces-c:

 xerces-c provides CMake targets:
 
     # this is heuristically generated, and may not be correct
     find_package(XercesC CONFIG REQUIRED)
-    target_link_libraries(main PRIVATE xerces_xerces-c XercesC::XercesC)
+    target_link_libraries(main PRIVATE XercesC::XercesC)

dg0yt · 2023-01-08T20:58:28Z

jsoncpp:

 jsoncpp provides CMake targets:
 
     # this is heuristically generated, and may not be correct
     find_package(jsoncpp CONFIG REQUIRED)
-    target_link_libraries(main PRIVATE jsoncpp_object jsoncpp_static JsonCpp::JsonCpp)
+    target_link_libraries(main PRIVATE JsonCpp::JsonCpp)

Neumann-A · 2023-01-08T21:19:13Z

src/vcpkg-test/install.cpp

+    auto with_var = get_cmake_add_library_names("add_library(bar${foo}) add_library(${foo}) add_library(   \nbar)");
+    CHECK(with_var.empty());
+
+    auto with_comment = get_cmake_add_library_names("add_library( # rem \n    foo)  add_library( bar# rem3)");


Suggested change

auto with_comment = get_cmake_add_library_names("add_library( # rem \n foo) add_library( bar# rem3)");

auto with_comment = get_cmake_add_library_names("add_library( # rem \n foo) add_library( bar # rem3\n)");

or should the target name be bar# ?

The point is to not return any target name at all from this input. It is not the generated form. If there is a comment, it likely that the heuristics won't generate useful output.

yeah but the input should still be valid cmake shouldn't it?

There is no benefit from perfection here. It is not a full parser.

Don't print targets when the config file is missing.

dg0yt · 2023-01-09T21:45:19Z

Last not least (microsoft/vcpkg#20190 (comment)):

-shapelib provides CMake targets:
-
-    # this is heuristically generated, and may not be correct
-    find_package(shp CONFIG REQUIRED)
-    target_link_libraries(main PRIVATE shp)
-

In particular, ignore everything in d(ebug) and all directory filepaths

dg0yt · 2023-01-10T18:53:21Z

Header-only output due to lack of config file for exports:

-mdns provides CMake targets:
-
-    # this is heuristically generated, and may not be correct
-    find_package(cmake CONFIG REQUIRED)
-    target_link_libraries(main PRIVATE mdns::mdns)
+mdns is header-only and can be used from CMake via:
+
+    find_path(MDNS_INCLUDE_DIRS "mdns.h")
+    target_include_directories(main PRIVATE ${MDNS_INCLUDE_DIRS})

BillyONeal

Only extreme nitpicks here; this looks good to me :)

BillyONeal · 2023-01-11T02:06:34Z

src/vcpkg/install.cpp

+        if (Strings::ends_with(filename, "Config.cmake"))
+        {
+            res = filename.substr(0, filename.size() - 12);
+        }
+        else if (Strings::ends_with(filename, "-config.cmake"))
+        {
+            res = filename.substr(0, filename.size() - 13);
+        }


I spent some time trying to get rid of the magic numbers. What do you think of:

// above namespace { bool remove_suffix(StringView& haystack, StringLiteral needle) { if (Strings::ends_with(haystack, needle)) { haystack = StringView{haystack.data(), haystack.size() - needle.size()}; return true; } return false; } }

Then:

Suggested change

if (Strings::ends_with(filename, "Config.cmake"))

{

res = filename.substr(0, filename.size() - 12);

}

else if (Strings::ends_with(filename, "-config.cmake"))

{

res = filename.substr(0, filename.size() - 13);

}

if (!remove_suffix(filename, "Config.cmake")) {

remove_suffix(filename, "-config.cmake");

}

(I'm not myself convinced that the cure is worse than the disease but thought I'd mention)

Will use StringLiteral.

BillyONeal · 2023-01-11T02:07:59Z

src/vcpkg/install.cpp

-                continue;
-            }
-            res.emplace_back(start_of_library_name, end_of_library_name);
+            first = end_of_library_name;


Suggested change

first = end_of_library_name;

first = end_of_library_name;

BillyONeal · 2023-01-11T02:08:38Z

src/vcpkg/install.cpp

@@ -740,122 +760,165 @@ namespace vcpkg
        auto files = fs.read_lines(installed.listfile_path(bpgh), ec);
        if (!ec)
        {
-            std::unordered_map<std::string, std::string> config_files;
+            // Mapping directories to unique config names and to library targets


This comment suggests that "name" should be "target"?

When I added the comment, it was for multiple lines. But the lines changed since that time.

BillyONeal · 2023-01-11T02:09:11Z

src/vcpkg/install.cpp

@@ -740,122 +760,165 @@ namespace vcpkg
        auto files = fs.read_lines(installed.listfile_path(bpgh), ec);
        if (!ec)
        {
-            std::unordered_map<std::string, std::string> config_files;
+            // Mapping directories to unique config names and to library targets
+            struct ConfigPackages


I'm personally not a fan of local structs like this but some other maintainers like them. Can it at least be moved out of the if?

The struct gives better names than first and second, and its location in the smallest possible scope. The alternative is to move it out of the function to an anonymous namespace.

I'm suggesting leaving it in the function but out of the if.

BillyONeal · 2023-01-11T20:19:21Z

src/vcpkg/install.cpp

            {
                auto msg = msg::format(msgCMakeTargetsUsage, msg::package_name = bpgh.spec.name()).append_raw("\n\n");
                msg.append_indent().append(msgCMakeTargetsUsageHeuristicMessage).append_raw('\n');

-                for (auto&& library_target_pair : library_targets)
+                for (const auto package_targets_pair : ret.cmake_targets_map)


Suggested change

for (const auto package_targets_pair : ret.cmake_targets_map)

for (auto&& package_targets_pair : ret.cmake_targets_map)

I believe auto&& as formerly written is more correct here: there's no reason to introduce an extra by-value copy.

Reverting. What I really wanted is a const reference. The block visits the container but must not modify it.

BillyONeal · 2023-01-13T22:49:57Z

Thanks!

dg0yt marked this pull request as draft January 8, 2023 17:45

dg0yt added 5 commits January 8, 2023 19:25

Stricter matching for add_library

3de92fe

Any deviation from the usual generated form calls for manual action.

Ignore what cannot be CMake config

ecacf19

Add proper cmake config package name detection

0b7d201

vcpkg wants config in 'share/${PORT}/' which maps to CMake's '<prefix>/(lib/<arch>|lib|share)/<name>*/' pattern, i.e. the beginning of the directory name must match the package name, case-insensitively.

Use new CMake config package detection

94268b6

Correlate package names and targets per 'share/${PORT}'. Ignore nested dirs.

Prefer namespaced targets

da2bf78

If both non-namespaced and namespaced targets exist, assume that the non-namespaced target is newer and preferred. Example: xerces_xerces-c -> XercesC::XercesC

dg0yt force-pushed the cmake-usage branch from 4d0f143 to da2bf78 Compare January 8, 2023 18:35

dg0yt marked this pull request as ready for review January 8, 2023 19:06

Neumann-A reviewed Jan 8, 2023

View reviewed changes

dg0yt added 5 commits January 9, 2023 07:43

Fix output logic

b49a758

Fix header-only logic

5f988c7

Match paths efficently

7fbfe5a

Do cmake data post-processing before message construction

1a3099d

Don't print targets when the config file is missing.

Avoid another std::map

cc9cd4f

Format and typo

72bf00d

dg0yt mentioned this pull request Jan 10, 2023

vcpkg install: Usage hint for CMake find_package, incomplete support for possible paths microsoft/vcpkg#10881

Closed

dg0yt added 3 commits January 10, 2023 09:24

More path evaluation short cuts

2f5477e

In particular, ignore everything in d(ebug) and all directory filepaths

Tweak

fd06498

Fix include path expression

ca44b66

dg0yt changed the title ~~Reduce broken output of cmake usage heuristics~~ Reduce invalid output of cmake usage heuristics Jan 10, 2023

dg0yt added 2 commits January 10, 2023 21:43

Tweak

e835c26

Format for rules, not for humans

0d983ca

BillyONeal approved these changes Jan 11, 2023

View reviewed changes

dg0yt added 2 commits January 12, 2023 08:23

CR changes

8388ec6

CI

6d62592

dg0yt added 4 commits January 12, 2023 22:33

Move ConfigPackages again

8961253

No magic length for include prefix

670cde3

Merge branch 'main' into cmake-usage

96b47c1

Format

b8102b9

BillyONeal merged commit fb588e9 into microsoft:main Jan 13, 2023

dg0yt deleted the cmake-usage branch January 14, 2023 04:54

MonicaLiu0311 mentioned this pull request Apr 20, 2023

[shaderc port] automatically adds 'SPIRV-Tools.lib' but it has wrong path at all microsoft/vcpkg#28144

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce invalid output of cmake usage heuristics #849

Reduce invalid output of cmake usage heuristics #849

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

Neumann-A Jan 8, 2023 •

edited

Loading

Neumann-A Jan 8, 2023

dg0yt Jan 9, 2023

Neumann-A Jan 9, 2023

dg0yt Jan 9, 2023

dg0yt commented Jan 9, 2023 •

edited

Loading

dg0yt commented Jan 10, 2023

BillyONeal left a comment

BillyONeal Jan 11, 2023

dg0yt Jan 12, 2023

BillyONeal Jan 11, 2023

BillyONeal Jan 11, 2023

dg0yt Jan 11, 2023

BillyONeal Jan 11, 2023

dg0yt Jan 11, 2023

BillyONeal Jan 12, 2023

BillyONeal Jan 11, 2023

dg0yt Jan 11, 2023

BillyONeal commented Jan 13, 2023

	auto with_comment = get_cmake_add_library_names("add_library( # rem \n foo) add_library( bar# rem3)");
	auto with_comment = get_cmake_add_library_names("add_library( # rem \n foo) add_library( bar # rem3\n)");

	for (const auto package_targets_pair : ret.cmake_targets_map)
	for (auto&& package_targets_pair : ret.cmake_targets_map)

Reduce invalid output of cmake usage heuristics #849

Reduce invalid output of cmake usage heuristics #849

Conversation

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

dg0yt commented Jan 8, 2023

Neumann-A Jan 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dg0yt commented Jan 9, 2023 • edited Loading

dg0yt commented Jan 10, 2023

BillyONeal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BillyONeal commented Jan 13, 2023

Neumann-A Jan 8, 2023 •

edited

Loading

dg0yt commented Jan 9, 2023 •

edited

Loading