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-fixup-cmake-targets] Rewrite the traversal code with -framework NAME on OSX #19629

Conversation

JackBoosY
Copy link
Contributor

@JackBoosY JackBoosY commented Aug 18, 2021

file(READ "${targets_file}" targets_content)
string(REGEX MATCHALL "INTERFACE_LINK_LIBRARIES[^\n]*\n" library_contents "${targets_content}")
foreach(line IN LISTS library_contents)

Because foreach (IN LISTS) separates each item according to a space, it incorrectly replaces the contents of non-INTERFACE_LINK_LIBRARIES. Such as:

set_target_properties(VTK::opengl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GL_SILENCE_DEPRECATION"
  INTERFACE_INCLUDE_DIRECTORIES "-framework OpenGL"
  INTERFACE_LINK_LIBRARIES "-framework OpenGL;-framework OpenGL"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "-framework OpenGL"
)

Fix that.

Now, this will fix

set_target_properties(VTK::opengl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GL_SILENCE_DEPRECATION"
  INTERFACE_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
  INTERFACE_LINK_LIBRARIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework;/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
)

to

set_target_properties(VTK::opengl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GL_SILENCE_DEPRECATION"
  INTERFACE_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
  INTERFACE_LINK_LIBRARIES "-framework OpenGL;-framework OpenGL"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
)

@JackBoosY JackBoosY added info:internal This PR or Issue was filed by the vcpkg team. category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:world-rebuild labels Aug 18, 2021
@JackBoosY JackBoosY changed the title [vcpkg baseline][vcpkg-fixup-cmake-targets] Add double quotes to `-fr… [vcpkg baseline][vcpkg-fixup-cmake-targets] Add double quotes to -framework NAMEon OSX Aug 18, 2021
@JackBoosY JackBoosY changed the title [vcpkg baseline][vcpkg-fixup-cmake-targets] Add double quotes to -framework NAMEon OSX [vcpkg baseline][vcpkg-fixup-cmake-targets] Add double quotes to -framework NAME on OSX Aug 18, 2021
@JackBoosY
Copy link
Contributor Author

cc @strega-nil-ms for review this PR.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/baseline.json b/versions/baseline.json
index 2855173..11f44b1 100644
--- a/versions/baseline.json
+++ b/versions/baseline.json
@@ -6617,7 +6617,7 @@
       "port-version": 0
     },
     "vcpkg-cmake-config": {
-      "baseline": "2021-08-11",
+      "baseline": "2021-08-18",
       "port-version": 0
     },
     "vcpkg-gfortran": {
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index affac6b..5ffdb7e 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,5 +1,10 @@
 {
   "versions": [
+    {
+      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "version-date": "2021-08-18",
+      "port-version": 0
+    },
     {
       "git-tree": "b3abb12ba8ab43770aea4e5a8d4915319bd295ee",
       "version-date": "2021-08-11",

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 5ffdb7e..26dfa00 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "0a1806a80ccf0f85fd4f79c5f2780adc4d92d78e",
       "version-date": "2021-08-18",
       "port-version": 0
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 5ffdb7e..26dfa00 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "0a1806a80ccf0f85fd4f79c5f2780adc4d92d78e",
       "version-date": "2021-08-18",
       "port-version": 0
     },

@Neumann-A
Copy link
Contributor

So this is what broke paraview

@dg0yt
Copy link
Contributor

dg0yt commented Aug 18, 2021

target_link_libraries(main PRIVATE -framework OpenGL)

How about

target_link_libraries(main PRIVATE OpenGL.framework)

instead of evil spaces?

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 5ffdb7e..fc78074 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "9f58eaa512e112b8a91d276b92a4dcbc52eef2ba",
       "version-date": "2021-08-18",
       "port-version": 0
     },

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 5ffdb7e..26dfa00 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "0a1806a80ccf0f85fd4f79c5f2780adc4d92d78e",
       "version-date": "2021-08-18",
       "port-version": 0
     },

@JackBoosY JackBoosY changed the title [vcpkg baseline][vcpkg-fixup-cmake-targets] Add double quotes to -framework NAME on OSX [vcpkg baseline][vcpkg-fixup-cmake-targets] Change the replace string with -framework NAME on OSX Aug 18, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 6b24f0fa76d171c5756f01f407682f7274cfe238 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 5ffdb7e..783aaa1 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "d4b5ea86aea3416a957fbe2384610feb6106f730",
       "version-date": "2021-08-18",
       "port-version": 0
     },

@JackBoosY JackBoosY marked this pull request as ready for review August 20, 2021 07:15
@@ -1,5 +1,10 @@
{
"versions": [
{
"git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
"git-tree": "d4b5ea86aea3416a957fbe2384610feb6106f730",

@JackBoosY JackBoosY added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Aug 20, 2021
@Neumann-A
Copy link
Contributor

INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"

Isn't there some kind of env variable the full path could be replaced with ? Because this changes doesn't change the fact that there are still full paths in the configs.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 20, 2021

INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"

Isn't there some kind of env variable the full path could be replaced with ? Because this changes doesn't change the fact that there are still full paths in the configs.

There is existing code that removes the path part for link libraries if it is in CMAKE_CXX_IMPLICIT_LINK_FRAMEWORK_DIRECTORIES

list(FIND VCPKG_DETECTED_CMAKE_CXX_IMPLICIT_LINK_FRAMEWORK_DIRECTORIES "${path}" index)

Should this be applied to include directories, too?

@JackBoosY
Copy link
Contributor Author

INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"

Isn't there some kind of env variable the full path could be replaced with ? Because this changes doesn't change the fact that there are still full paths in the configs.

I wish too, but I didn't find that.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

This is a new experimental fast check for PR issues. Please let us know if this bot is helpful!

After committing all other changes, the version database must be updated
git add -u && git commit
git checkout 025e564979cc01d0fbc5c920aa8a36635efb01bb -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/v-/vcpkg-cmake-config.json b/versions/v-/vcpkg-cmake-config.json
index 042a2dc..ed1308b 100644
--- a/versions/v-/vcpkg-cmake-config.json
+++ b/versions/v-/vcpkg-cmake-config.json
@@ -1,15 +1,10 @@
 {
   "versions": [
     {
-      "git-tree": "dd7d67c81c2442c831a298d136b019a2bd138565",
+      "git-tree": "fddc25466db06c2b22135793bce00f8c482a338c",
       "version-date": "2021-08-18",
       "port-version": 0
     },
-    {
-      "git-tree": "b3abb12ba8ab43770aea4e5a8d4915319bd295ee",
-      "version-date": "2021-08-11",
-      "port-version": 0
-    },
     {
       "git-tree": "330cc51bc99c6b71ed5fb51901f6f838684015a5",
       "version-date": "2021-05-22",

@JackBoosY
Copy link
Contributor Author

INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"

Isn't there some kind of env variable the full path could be replaced with ? Because this changes doesn't change the fact that there are still full paths in the configs.

There is existing code that removes the path part for link libraries if it is in CMAKE_CXX_IMPLICIT_LINK_FRAMEWORK_DIRECTORIES

list(FIND VCPKG_DETECTED_CMAKE_CXX_IMPLICIT_LINK_FRAMEWORK_DIRECTORIES "${path}" index)

Should this be applied to include directories, too?

No, the original error was caused by changing the value of *_INCLUDE_DIRECTORIES to -framework VALUE.

@dg0yt
Copy link
Contributor

dg0yt commented Aug 23, 2021

No, the original error was caused by changing the value of *_INCLUDE_DIRECTORIES to -framework VALUE.

Sure.

I am concerned that INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework" contains an absolute path from the port build environment, and it might limit the usability of the binary cache items if the consuming environment uses another path for the SDK.

@cenit
Copy link
Contributor

cenit commented Aug 23, 2021

i agree with @dg0yt and @Neumann-A

this is a half-backed fix, because there are still absolute paths. Why fixing only some? We still have the problems of having absolute paths in the configs! Better to fix none in the end, instead of a half-solution?

so to say: what is this PR fixing, in the end?

@JackBoosY JackBoosY changed the title [vcpkg baseline][vcpkg-fixup-cmake-targets] Rewrite the traversal code with -framework NAME on OSX [vcpkg-fixup-cmake-targets] Rewrite the traversal code with -framework NAME on OSX Aug 24, 2021
@JackBoosY
Copy link
Contributor Author

JackBoosY commented Aug 24, 2021

@cenit See #16259, the origin PR wants to replace those absolute system paths in cmake files.

@cenit
Copy link
Contributor

cenit commented Aug 24, 2021

@cenit See #16259, the origin PR wants to replace those absolute system paths in cmake files.

yeah I know.
But why leaving absolute paths in INTERFACE_INCLUDE_DIRECTORIES and INTERFACE_SYSTEM_INCLUDE_DIRECTORIES ? Since you added -framework properly, those lines might even be removed if they do not contain any additional library.
Leaving them with an absolute path is just asking for troubles in future (portability and binary cache are definitely broken with those absolute paths left behind)!

@dg0yt
Copy link
Contributor

dg0yt commented Aug 24, 2021

We know how to the substitution now, and we know the ports to look at for verification. Do it right and completely.
(We might immediately add a hard check which just blindly searches for known build system path strings in ...config.cmake files, .pc files and ...-config scripts.)

@JackBoosY
Copy link
Contributor Author

@dg0yt But we don't know which expression should be replaced.

@Neumann-A
Copy link
Contributor

@JackBoosY:

(/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/[^;"]+) ? + maybe a ;; and ;" replacement

@cenit
Copy link
Contributor

cenit commented Aug 24, 2021

@dg0yt But we don't know which expression should be replaced.

nothing, it's not necessary if you have a proper "-framework VALUE" in the interface_link_library slot.
You can remove them from INTERFACE_INCLUDE_DIRECTORIES and INTERFACE_SYSTEM_INCLUDE_DIRECTORIES and it should work, without having to fix them! Did you try this way?

@cenit
Copy link
Contributor

cenit commented Aug 24, 2021

Of course, before removing everything from those fields, you have to be sure there are not other libraries, which are not part of the "fixed" frameworks included in the fixed "interface_link_libraries" line ;)

@cenit
Copy link
Contributor

cenit commented Aug 24, 2021

so to say

set_target_properties(VTK::opengl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GL_SILENCE_DEPRECATION"
  INTERFACE_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
  INTERFACE_LINK_LIBRARIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework;/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
  INTERFACE_SYSTEM_INCLUDE_DIRECTORIES "/Library/Developer/CommandLineTools/SDKs/MacOSX11.3.sdk/System/Library/Frameworks/OpenGL.framework"
)

should become

set_target_properties(VTK::opengl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GL_SILENCE_DEPRECATION"
  INTERFACE_LINK_LIBRARIES "-framework OpenGL"
)

@PhoebeHui PhoebeHui marked this pull request as draft August 26, 2021 06:10
@PhoebeHui PhoebeHui removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Sep 14, 2021
@JackBoosY JackBoosY mentioned this pull request Oct 8, 2021
1 task
@JackBoosY
Copy link
Contributor Author

Temporary close this issue because I have no good idea about that.

@JackBoosY JackBoosY closed this Nov 23, 2021
@cenit
Copy link
Contributor

cenit commented Nov 23, 2021

@autoantwort how did you manage this issue in your “remove absolute paths” heroic effort?

@autoantwort
Copy link
Contributor

The absolute path check only checks for absolute paths to the package and the installed dir paths.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly info:internal This PR or Issue was filed by the vcpkg team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants