[vcpkg_fixup_pkgconfig] Check for more problems, add unit test (#23898)

* Print stack traces for failed unit tests

* Add test for vcpkg_fixup_pkgconfig.cmake

* Check for 'optimized' and 'debug' in pc files

* Check for 'NOTFOUND' and for 'ns::target' in pc files

* Check for, and fix, line continuations

* Test file path substitutions

* Move contents processing into separate function

* Update and leverage line break normalization

* Pass prefix to data processing

* Validate only the collapsed Libs

* Test removal of '<field>.private'

* Declare unit test license (same as vcpkg)

* Replace ';' with ' ' in 'Libs:'

* Disambiguate parameter variable names

* Test quoting of variables

* Fix quoting of variables

* Quote whole parameters

* Process and quote libs item-wise

* Resolve keywords 'optimized', 'debug', 'debug'

* Consistency

* CI [skip actions]

* CI [skip actions]

* Don't fail on detected errors

* Disable unit-testing for fatal errors

Co-authored-by: Billy O'Neal <bion@microsoft.com>

scripts/cmake/vcpkg_fixup_pkgconfig.cmake

@@ -1,4 +1,108 @@
-function(z_vcpkg_fixup_pkgconfig_check_files file config)
+function(z_vcpkg_fixup_pkgconfig_process_data arg_variable arg_config arg_prefix)
+    # This normalizes all data to start and to end with a newline, and
+    # to use LF instead of CRLF. This allows to use simpler regex matches.
+    string(REPLACE "\r\n" "\n" contents "\n${${arg_variable}}\n")
+
+    string(REPLACE "${CURRENT_PACKAGES_DIR}" [[${prefix}]] contents "${contents}")
+    string(REPLACE "${CURRENT_INSTALLED_DIR}" [[${prefix}]] contents "${contents}")
+    if(VCPKG_HOST_IS_WINDOWS)
+        string(REGEX REPLACE "^([a-zA-Z]):/" [[/\1/]] unix_packages_dir "${CURRENT_PACKAGES_DIR}")
+        string(REPLACE "${unix_packages_dir}" [[${prefix}]] contents "${contents}")
+        string(REGEX REPLACE "^([a-zA-Z]):/" [[/\1/]] unix_installed_dir "${CURRENT_INSTALLED_DIR}")
+        string(REPLACE "${unix_installed_dir}" [[${prefix}]] contents "${contents}")
+    endif()
+
+    string(REGEX REPLACE "\n[\t ]*prefix[\t ]*=[^\n]*" "" contents "prefix=${arg_prefix}${contents}")
+    if("${arg_config}" STREQUAL "DEBUG")
+        # prefix points at the debug subfolder
+        string(REPLACE [[${prefix}/debug]] [[${prefix}]] contents "${contents}")
+        string(REPLACE [[${prefix}/include]] [[${prefix}/../include]] contents "${contents}")
+        string(REPLACE [[${prefix}/share]] [[${prefix}/../share]] contents "${contents}")
+    endif()
+    # Remove line continuations before transformations
+    string(REGEX REPLACE "[ \t]*\\\\\n[ \t]*" " " contents "${contents}")
+    # This section fuses XYZ.private and XYZ according to VCPKG_LIBRARY_LINKAGE
+    #
+    # Pkgconfig searches Requires.private transitively for Cflags in the dynamic case,
+    # which prevents us from removing it.
+    #
+    # Once this transformation is complete, users of vcpkg should never need to pass
+    # --static.
+    if("${VCPKG_LIBRARY_LINKAGE}" STREQUAL "static")
+        # how this works:
+        # we want to transform:
+        #   Libs: $1
+        #   Libs.private: $2
+        # into
+        #    Libs: $1 $2
+        # and the same thing for Requires and Requires.private
+
+        foreach(item IN ITEMS "Libs" "Requires" "Cflags")
+            set(line "")
+            if("${contents}" MATCHES "\n${item}: *([^\n]*)")
+                string(APPEND line " ${CMAKE_MATCH_1}")
+            endif()
+            if("${contents}" MATCHES "\n${item}\\.private: *([^\n]*)")
+                string(APPEND line " ${CMAKE_MATCH_1}")
+            endif()
+
+            string(REGEX REPLACE "\n${item}(\\.private)?:[^\n]*" "" contents "${contents}")
+            if(NOT "${line}" STREQUAL "")
+                string(APPEND contents "${item}:${line}\n")
+            endif()
+        endforeach()
+    endif()
+
+    if(contents MATCHES "\nLibs: *([^\n]*)")
+        set(libs "${CMAKE_MATCH_1}")
+        if(libs MATCHES [[;]])
+            # Assuming that ';' comes from CMake lists only. Candidate for parameter control.
+            string(REPLACE ";" " " no_lists "${libs}")
+            string(REPLACE "${libs}" "${no_lists}" contents "${contents}")
+            set(libs "${no_lists}")
+        endif()
+
+        separate_arguments(libs_list UNIX_COMMAND "${libs}")
+        set(skip_next 0)
+        set(libs_filtered "")
+        foreach(item IN LISTS libs_list)
+            if(skip_next)
+                set(skip_next 0)
+                continue()
+            elseif(item MATCHES "^(-l|-L)?optimized")
+                string(COMPARE EQUAL "${arg_config}" "DEBUG" skip_next)
+                continue()
+            elseif(item MATCHES "^(-l|-L)?debug")
+                string(COMPARE EQUAL "${arg_config}" "RELEASE" skip_next)
+                continue()
+            elseif(item MATCHES "^(-l|-L)?general")
+                continue()
+            endif()
+            if(item MATCHES "[\$`\"\\ ]")
+                set(item "\"${item}\"")
+            endif()
+            list(APPEND libs_filtered "${item}")
+        endforeach()
+        list(JOIN libs_filtered " " libs_filtered)
+        string(REPLACE "${libs}" "${libs_filtered}" contents "${contents}")
+        set(libs "${libs_filtered}")
+
+        if(libs MATCHES "[^ ]*-NOTFOUND")
+            message(WARNING "Error in ${file}: 'Libs' refers to a missing lib:\n...${CMAKE_MATCH_0}")
+        endif()
+        if(libs MATCHES "[^\n]*::[^\n ]*")
+            message(WARNING "Error in ${file}: 'Libs' refers to a CMake target:\n...${CMAKE_MATCH_0}")
+        endif()
+    endif()
+
+    # Quote -L, -I, and -l paths starting with `${blah}`
+    # This was already handled for "Libs", but there might be additional occurrences in other lines.
+    string(REGEX REPLACE "([ =])(-[LIl]\\\${[^}]*}[^ ;\n\t]*)" [[\1"\2"]] contents "${contents}")
+
+    set("${arg_variable}" "${contents}" PARENT_SCOPE)
+endfunction()
+
+function(z_vcpkg_fixup_pkgconfig_check_files arg_file arg_config)
     set(path_suffix_DEBUG /debug)
     set(path_suffix_RELEASE "")
 
@@ -9,15 +113,15 @@ function(z_vcpkg_fixup_pkgconfig_check_files file config)
     endif()
 
     vcpkg_host_path_list(PREPEND ENV{PKG_CONFIG_PATH}
-        "${CURRENT_PACKAGES_DIR}${path_suffix_${config}}/lib/pkgconfig"
+        "${CURRENT_PACKAGES_DIR}${path_suffix_${arg_config}}/lib/pkgconfig"
         "${CURRENT_PACKAGES_DIR}/share/pkgconfig"
-        "${CURRENT_INSTALLED_DIR}${path_suffix_${config}}/lib/pkgconfig"
+        "${CURRENT_INSTALLED_DIR}${path_suffix_${arg_config}}/lib/pkgconfig"
         "${CURRENT_INSTALLED_DIR}/share/pkgconfig"
     )
 
     # First make sure everything is ok with the package and its deps
-    cmake_path(GET file STEM LAST_ONLY package_name)
-    debug_message("Checking package (${config}): ${package_name}")
+    cmake_path(GET arg_file STEM LAST_ONLY package_name)
+    debug_message("Checking package (${arg_config}): ${package_name}")
     execute_process(
         COMMAND "${PKGCONFIG}" --print-errors --exists "${package_name}"
         WORKING_DIRECTORY "${CURRENT_BUILDTREES_DIR}"
@@ -68,14 +172,8 @@ function(vcpkg_fixup_pkgconfig)
         endforeach()
     endif()
 
-    string(REGEX REPLACE "^([a-zA-Z]):/" [[/\1/]] unix_packages_dir "${CURRENT_PACKAGES_DIR}")
-    string(REGEX REPLACE "^([a-zA-Z]):/" [[/\1/]] unix_installed_dir "${CURRENT_INSTALLED_DIR}")
-
     foreach(config IN ITEMS RELEASE DEBUG)
         debug_message("${config} Files: ${arg_${config}_FILES}")
-        if("${VCPKG_BUILD_TYPE}" STREQUAL "debug" AND "${config}" STREQUAL "RELEASE")
-            continue()
-        endif()
         if("${VCPKG_BUILD_TYPE}" STREQUAL "release" AND "${config}" STREQUAL "DEBUG")
             continue()
         endif()
@@ -91,60 +189,8 @@ function(vcpkg_fixup_pkgconfig)
             endif()
             #Correct *.pc file
             file(READ "${file}" contents)
-
-            # this normalizes all files to end with a newline, and use LF instead of CRLF;
-            # this allows us to use regex matches easier to modify these files.
-            if(NOT "${contents}" MATCHES "\n$")
-                string(APPEND contents "\n")
-            endif()
-            string(REPLACE "\r\n" "\n" contents "${contents}")
-
-            string(REPLACE "${CURRENT_PACKAGES_DIR}" [[${prefix}]] contents "${contents}")
-            string(REPLACE "${CURRENT_INSTALLED_DIR}" [[${prefix}]] contents "${contents}")
-            string(REPLACE "${unix_packages_dir}" [[${prefix}]] contents "${contents}")
-            string(REPLACE "${unix_installed_dir}" [[${prefix}]] contents "${contents}")
-
-            string(REGEX REPLACE "(^|\n) *prefix[\t ]*=[^\n]*" "" contents "${contents}")
-            if("${config}" STREQUAL "DEBUG")
-                # prefix points at the debug subfolder
-                string(REPLACE [[${prefix}/debug]] [[${prefix}]] contents "${contents}")
-                string(REPLACE [[${prefix}/include]] [[${prefix}/../include]] contents "${contents}")
-                string(REPLACE [[${prefix}/share]] [[${prefix}/../share]] contents "${contents}")
-            endif()
-            # quote -L, -I, and -l paths starting with `${blah}`
-            string(REGEX REPLACE " -([LIl])(\\\${[^}]*}[^ \n\t]*)" [[ -\1"\2"]] contents "${contents}")
-            # This section fuses XYZ.private and XYZ according to VCPKG_LIBRARY_LINKAGE
-            #
-            # Pkgconfig searches Requires.private transitively for Cflags in the dynamic case,
-            # which prevents us from removing it.
-            #
-            # Once this transformation is complete, users of vcpkg should never need to pass
-            # --static.
-            if("${VCPKG_LIBRARY_LINKAGE}" STREQUAL "static")
-                # how this works:
-                # we want to transform:
-                #   Libs: $1
-                #   Libs.private: $2
-                # into
-                #    Libs: $1 $2
-                # and the same thing for Requires and Requires.private
-
-                foreach(item IN ITEMS "Libs" "Requires" "Cflags")
-                    set(line "")
-                    if("${contents}" MATCHES "(^|\n)${item}: *([^\n]*)")
-                        string(APPEND line " ${CMAKE_MATCH_2}")
-                    endif()
-                    if("${contents}" MATCHES "(^|\n)${item}\\.private: *([^\n]*)")
-                        string(APPEND line " ${CMAKE_MATCH_2}")
-                    endif()
-
-                    string(REGEX REPLACE "(^|\n)${item}(\\.private)?:[^\n]*\n" [[\1]] contents "${contents}")
-                    if(NOT "${line}" STREQUAL "")
-                        string(APPEND contents "${item}:${line}\n")
-                    endif()
-                endforeach()
-            endif()
-            file(WRITE "${file}" "prefix=\${pcfiledir}/${relative_pc_path}\n${contents}")
+            z_vcpkg_fixup_pkgconfig_process_data(contents "${config}" "\${pcfiledir}/${relative_pc_path}")
+            file(WRITE "${file}" "${contents}")
         endforeach()
 
         if(NOT arg_SKIP_CHECK) # The check can only run after all files have been corrected!

scripts/test_ports/unit-test-cmake/portfile.cmake

@@ -44,9 +44,9 @@ function(unit_test_check_variable_unset utcvu_test utcvu_variable)
     if(Z_VCPKG_UNIT_TEST_HAS_FATAL_ERROR)
         unset_fatal_error()
         set_has_error()
-        message(STATUS "${utcvu_test} had an unexpected FATAL_ERROR;
+        message(SEND_ERROR "${utcvu_test} had an unexpected FATAL_ERROR;
     expected: \"${utcvu_value}\"")
-        message(STATUS "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
+        message(SEND_ERROR "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
         return()
     endif()
 
@@ -65,7 +65,7 @@ function(unit_test_check_variable_unset utcvu_test utcvu_variable)
     endif()
 
     if(DEFINED "${utcvu_variable}")
-        message(STATUS "${utcvu_test} set ${utcvu_variable};
+        message(SEND_ERROR "${utcvu_test} set ${utcvu_variable};
     expected: \"${utcvu_variable}\" unset
     actual  : \"${utcvu_actual_value}\"")
         set_has_error()
@@ -78,14 +78,14 @@ function(unit_test_check_variable_equal utcve_test utcve_variable utcve_value)
     if(Z_VCPKG_UNIT_TEST_HAS_FATAL_ERROR)
         unset_fatal_error()
         set_has_error()
-        message(STATUS "${utcve_test} had an unexpected FATAL_ERROR;
+        message(SEND_ERROR "${utcve_test} had an unexpected FATAL_ERROR;
     expected: \"${utcve_value}\"")
-        message(STATUS "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
+        message(SEND_ERROR "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
         return()
     endif()
 
     if(NOT DEFINED "${utcve_variable}" AND NOT "${utcve_variable}" MATCHES "^ENV\\{")
-        message(STATUS "${utcve_test} failed to set ${utcve_variable};
+        message(SEND_ERROR "${utcve_test} failed to set ${utcve_variable};
     expected: \"${utcve_value}\"")
         set_has_error()
         return()
@@ -105,7 +105,7 @@ function(unit_test_check_variable_equal utcve_test utcve_variable utcve_value)
     endif()
 
     if(NOT "${utcve_actual_value}" STREQUAL "${utcve_value}")
-        message(STATUS "${utcve_test} resulted in the wrong value for ${utcve_variable};
+        message(SEND_ERROR "${utcve_test} resulted in the wrong value for ${utcve_variable};
     expected: \"${utcve_value}\"
     actual  : \"${utcve_actual_value}\"")
         set_has_error()
@@ -118,9 +118,9 @@ function(unit_test_check_variable_not_equal utcve_test utcve_variable utcve_valu
     if(Z_VCPKG_UNIT_TEST_HAS_FATAL_ERROR)
         unset_fatal_error()
         set_has_error()
-        message(STATUS "${utcve_test} had an unexpected FATAL_ERROR;
+        message(SEND_ERROR "${utcve_test} had an unexpected FATAL_ERROR;
     expected: \"${utcve_value}\"")
-        message(STATUS "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
+        message(SEND_ERROR "FATAL_ERROR: ${Z_VCPKG_UNIT_TEST_FATAL_ERROR}")
         return()
     endif()
 
@@ -138,7 +138,7 @@ function(unit_test_check_variable_not_equal utcve_test utcve_variable utcve_valu
     endif()
 
     if("${utcve_actual_value}" STREQUAL "${utcve_value}")
-        message(STATUS "${utcve_test} failed to change ${utcve_variable};
+        message(SEND_ERROR "${utcve_test} failed to change ${utcve_variable};
     unchanged: \"${utcve_value}\"")
         set_has_error()
         return()
@@ -149,15 +149,15 @@ function(unit_test_ensure_success utcve_test)
     cmake_language(EVAL CODE "${utcve_test}")
     if(Z_VCPKG_UNIT_TEST_HAS_FATAL_ERROR)
         set_has_error()
-        message(STATUS "${utcve_test} was expected to be successful.")
+        message(SEND_ERROR "${utcve_test} was expected to be successful.")
     endif()
     unset_fatal_error()
 endfunction()
 function(unit_test_ensure_fatal_error utcve_test)
     cmake_language(EVAL CODE "${utcve_test}")
     if(NOT Z_VCPKG_UNIT_TEST_HAS_FATAL_ERROR)
         set_has_error()
-        message(STATUS "${utcve_test} was expected to be a FATAL_ERROR.")
+        message(SEND_ERROR "${utcve_test} was expected to be a FATAL_ERROR.")
     endif()
     unset_fatal_error()
 endfunction()
@@ -185,6 +185,9 @@ endif()
 if("setup-pkgconfig-path" IN_LIST FEATURES)
     include("${CMAKE_CURRENT_LIST_DIR}/test-z_vcpkg_setup_pkgconfig_path.cmake")
 endif()
+if("fixup-pkgconfig" IN_LIST FEATURES)
+    include("${CMAKE_CURRENT_LIST_DIR}/test-vcpkg_fixup_pkgconfig.cmake")
+endif()
 
 if(Z_VCPKG_UNIT_TEST_HAS_ERROR)
     _message(FATAL_ERROR "At least one test failed")