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

[vcpkg-cmake-config] Fix fixup of ) paths #38921

Merged
merged 3 commits into from
Jun 6, 2024
Merged

Conversation

dg0yt
Copy link
Contributor

@dg0yt dg0yt commented May 24, 2024

Cherry-picked from #38847:

Fix processing of INTERFACE_LINK_LIBS when it contain ) inside ", e.g. for
C:/Program Files (x86)/Windows Kits/10/Lib/10.0.22621.0/um/x64/User32.Lib
created by pkg_check_modules from -luser32 as found in icu pc files.

Without the fix, the property is not processed at all, pulling release libs into debug builds.

@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label May 24, 2024
@Neumann-A
Copy link
Contributor

C:/Program Files (x86)/Windows Kits/10/Lib/10.0.22621.0/um/x64/ needs to be removed

@dg0yt
Copy link
Contributor Author

dg0yt commented May 27, 2024

C:/Program Files (x86)/Windows Kits/10/Lib/10.0.22621.0/um/x64/ needs to be removed

Actually this PR deals only with completely broken results due to ), not with removing SDL paths. It fixes immediate regressions in vcpkg CI as observed in #33847.

category:port-bug The issue is with a library, which is something the port should already support

@dg0yt dg0yt changed the title [vcpkg-cmake-config] Fix fixup of (x86) paths [vcpkg-cmake-config] Fix fixup of ) paths May 27, 2024
@dg0yt
Copy link
Contributor Author

dg0yt commented May 27, 2024

This PR changes CMake config, typically fixing the debug variant. Why does the angle build always get stuck in the windows release build phase?

@talregev
Copy link
Contributor

@dg0yt Please try to merge the latest dev.
@BillyONeal I also see the ci get stuck on that changes. Can you take a look?

@dg0yt
Copy link
Contributor Author

dg0yt commented May 27, 2024

Please try to merge the latest dev.

Pointless. Vcpkg CI build merges with master.

AT BillyONeal I also see the ci get stuck on that changes. Can you take a look?

@talregev I don't need a proxy in my vcpkg contacts.

FTR it is not the build step which is stuck but the cmake config fixup which follows. Which is the code changed by my PR.

@dg0yt dg0yt marked this pull request as draft May 27, 2024 11:04
@dg0yt dg0yt marked this pull request as draft May 27, 2024 11:04
@talregev
Copy link
Contributor

talregev commented May 27, 2024

In my opinion the ci stuck not from your code change.
I already see the ci stuck, usually trying to build heavy things, or it needs to compile many ports. It also happened on osx many times.

And we already have a ci problems so I am asking to check that.

@dg0yt
Copy link
Contributor Author

dg0yt commented May 28, 2024

Regular expression woes. Escaped double quotes inside quotes in INTERFACE_COMPILE_DEFINITIONS here, make CMake hang in the while expression. Reproducer:

set(contents [[
# Generated CMake target import file.
add_library(unofficial::angle::libANGLE STATIC IMPORTED)
set_target_properties(unofficial::angle::libANGLE PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "ANGLE_ENABLE_ESSL;ANGLE_ENABLE_GLSL;ANGLE_CAPTURE_ENABLED=0;GL_APICALL=;GL_API=;NOMINMAX;ANGLE_ENABLE_D3D11;ANGLE_ENABLE_HLSL;ANGLE_PRELOADED_D3DCOMPILER_MODULE_NAMES={ \"d3dcompiler_47.dll\", \"d3dcompiler_46.dll\", \"d3dcompiler_43.dll\" };ANGLE_ENABLE_D3D9;ANGLE_ENABLE_D3D11_COMPOSITOR_NATIVE_WINDOW;ANGLE_ENABLE_GLSL;ANGLE_ENABLE_OPENGL;ANGLE_ENABLE_GL_DESKTOP_BACKEND;KHRONOS_STATIC"
  INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:ZLIB::ZLIB>"
)
]])
while(contents MATCHES "set_target_properties[(]([^ \$]*) PROPERTIES([^\")]+| \"[^\"]*\")+[)]")
    message(STATUS "FOUND set_target_properties")
endwhile()
message(STATUS "${contents}")

The function is already only touching files which can be assumed to be generated by CMake. Maybe we should simply use this to more closely match on an INTERFACE_LINK_LIBRARIES "..."\n line.

@dg0yt dg0yt force-pushed the cmake-config branch 2 times, most recently from 2e5b3d6 to 92ec7af Compare May 31, 2024 04:32
@dg0yt dg0yt force-pushed the cmake-config branch 2 times, most recently from 2f0a27a to b134218 Compare June 1, 2024 07:22
@dg0yt dg0yt marked this pull request as ready for review June 1, 2024 07:22
@talregev
Copy link
Contributor

talregev commented Jun 4, 2024

@FrankXie05 can you take a look?

@FrankXie05 FrankXie05 added category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines and removed category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist labels Jun 5, 2024
@vicroms vicroms merged commit 233f096 into microsoft:master Jun 6, 2024
17 checks passed
@dg0yt dg0yt deleted the cmake-config branch June 6, 2024 15:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants