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] Meson osx sysroot #21772

Merged
merged 6 commits into from
Feb 24, 2022
Merged

Conversation

daschuer
Copy link
Contributor

@daschuer daschuer commented Nov 30, 2021

This allows to use VCPKG_OSX_SYSROOT with meson ports. This is required to crosscompile formatconfig for arm64 on osx. Shown here: https://github.com/daschuer/vcpkg/runs/4363816948?check_suite_focus=true

  • What does your PR fix?

    Fixes ignoring VCPKG_OSX_SYSROOT and VCPKG_OSX_ARCHITECTURES when linking meson projects.

This also removes a deprectaion warning and reformats io2d a ffmpeg vcpkg.json to allow updating the version database

  • Which triplets are supported/not supported? Have you updated the CI baseline?

No change

  • Does your PR follow the maintainer guide?

    Yes

  • If you have added/updated a port: Have you run ./vcpkg x-add-version --all and committed the result?

    Yes

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!

All manifest files must be formatted

./vcpkg format-manifest ports/*/vcpkg.json

Diff
diff --git a/ports/ffmpeg/vcpkg.json b/ports/ffmpeg/vcpkg.json
index 6f60bce..ecfe5e3 100644
--- a/ports/ffmpeg/vcpkg.json
+++ b/ports/ffmpeg/vcpkg.json
@@ -133,25 +133,25 @@
           "name": "ffmpeg",
           "default-features": false,
           "features": [
-            "ssh"
+            "dav1d"
           ],
-          "platform": "!(uwp | arm) & !static"
+          "platform": "!(uwp | arm | x86 | osx)"
         },
         {
           "name": "ffmpeg",
           "default-features": false,
           "features": [
-            "opengl"
+            "ssh"
           ],
-          "platform": "!uwp & !(windows & arm)"
+          "platform": "!(uwp | arm) & !static"
         },
         {
           "name": "ffmpeg",
           "default-features": false,
           "features": [
-            "dav1d"
+            "opengl"
           ],
-          "platform": "!(uwp | arm | x86 | osx)"
+          "platform": "!uwp & !(windows & arm)"
         },
         {
           "name": "ffmpeg",
diff --git a/ports/io2d/vcpkg.json b/ports/io2d/vcpkg.json
index 5b494a2..cbe8569 100644
--- a/ports/io2d/vcpkg.json
+++ b/ports/io2d/vcpkg.json
@@ -4,10 +4,6 @@
   "port-version": 4,
   "description": "a lightweight, cross platform drawing library",
   "dependencies": [
-    {
-      "name": "cairo",
-      "platform": "!osx"
-    },
     {
       "name": "cairo",
       "features": [
@@ -15,6 +11,10 @@
       ],
       "platform": "linux"
     },
+    {
+      "name": "cairo",
+      "platform": "!osx"
+    },
     {
       "name": "graphicsmagick",
       "platform": "!osx"
After committing all other changes, the version database must be updated
git add -u && git commit
git checkout c1c6eb92e91b99ca906016499d17088a32137824 -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/f-/ffmpeg.json b/versions/f-/ffmpeg.json
index 580a897..4a670fe 100644
--- a/versions/f-/ffmpeg.json
+++ b/versions/f-/ffmpeg.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "d18225e2ca707165bee0288fe9f1ce59f45b0611",
+      "git-tree": "ca9c704e55efefe1598499ac7c73ec6127d5d3b8",
       "version": "4.4.1",
       "port-version": 2
     },
diff --git a/versions/i-/io2d.json b/versions/i-/io2d.json
index cb6b102..c35f949 100644
--- a/versions/i-/io2d.json
+++ b/versions/i-/io2d.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "5aa97bd603c2f6e4f9e79a9d3317b37de833dad0",
+      "git-tree": "120191e188ace7a75192ff41e715f39ddd05a179",
       "version-date": "2020-09-14",
       "port-version": 4
     },

@JonLiu1993 JonLiu1993 added the category:port-bug The issue is with a library, which is something the port should already support label Dec 1, 2021
@JonLiu1993
Copy link
Member

@PhoebeHui PhoebeHui changed the title Meson osx sysroot [vcpkg] Meson osx sysroot Dec 2, 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 5ddd7f02689b7c5aab78711d77f61db5d2e5e79c -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/f-/fontconfig.json b/versions/f-/fontconfig.json
index 86b2b1e..14da018 100644
--- a/versions/f-/fontconfig.json
+++ b/versions/f-/fontconfig.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "bbe3d54f98efcd8741302799dd5c05cd9d4e19d7",
+      "git-tree": "596967876e9619f66e0bd5e2e58a8bc6c23d8ca8",
       "version": "2.13.94",
       "port-version": 4
     },

@JackBoosY JackBoosY added category:tool-update The issue is with build tool or build script, which requires update or should be executed correctly and removed category:port-bug The issue is with a library, which is something the port should already support labels Dec 2, 2021
@JackBoosY
Copy link
Contributor

cc @Neumann-A for simply review this PR.

Comment on lines 155 to 192
if(VCPKG_TARGET_IS_OSX)
# macOS - append arch and isysroot if cross-compiling
if(NOT "${VCPKG_OSX_ARCHITECTURES}" STREQUAL "${VCPKG_DETECTED_CMAKE_HOST_SYSTEM_PROCESSOR}")
foreach(arch IN LISTS VCPKG_OSX_ARCHITECTURES)
vcpkg_list(APPEND linker_flags -arch "${arch}")
endforeach()
endif()
if(VCPKG_OSX_SYSROOT)
vcpkg_list(APPEND linker_flags -isysroot "${VCPKG_OSX_SYSROOT}")
endif()
endif()
Copy link
Contributor

@Neumann-A Neumann-A Dec 2, 2021

Choose a reason for hiding this comment

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

the flags should come from CMake instead of being manually added here. (because autotools probably also require them)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The linker flag is already obtained form cmake, see line 152. Unfortunately it does not contain the -sysroot and the -arch parameters. AFAIK, the autotool ports are already working. That is why I have decided for this solution with a limited scope.

Copy link
Contributor Author

@daschuer daschuer Dec 2, 2021

Choose a reason for hiding this comment

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

I have just double checked the port flatbuffers here
https://github.com/daschuer/vcpkg/actions/runs/1531891431

It already uses -arch and -isysroot as desired. My guess is that the autotools are smart enough to take them form the cxxflags when linking:

37/38] : && /Applications/Xcode_12.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/c++ -fPIC -mmacosx-version-min=11.0 -std=c++11 -stdlib=libc++ -Wall -pedantic -Werror -Wextra -Wno-unused-parameter -O3 -DNDEBUG -arch arm64 -isysroot /Applications/Xcode_12.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX11.0.sdk -mmacosx-version-min=11.0 -Wl,-search_paths_first -Wl,-headerpad_max_install_names  CMakeFiles/flatc.dir/src/idl_parser.cpp.o CMakeFiles/flatc.dir/src/idl_gen_text.cpp.o CMakeFiles/flatc.dir/src/reflection.cpp.o CMakeFiles/flatc.dir/src/util.cpp.o CMakeFiles/flatc.dir/src/idl_gen_cpp.cpp.o CMakeFiles/flatc.dir/src/idl_gen_csharp.cpp.o CMakeFiles/flatc.dir/src/idl_gen_dart.cpp.o CMakeFiles/flatc.dir/src/idl_gen_kotlin.cpp.o CMakeFiles/flatc.dir/src/idl_gen_go.cpp.o CMakeFiles/flatc.dir/src/idl_gen_java.cpp.o CMakeFiles/flatc.dir/src/idl_gen_ts.cpp.o CMakeFiles/flatc.dir/src/idl_gen_php.cpp.o CMakeFiles/flatc.dir/src/idl_gen_python.cpp.o CMakeFiles/flatc.dir/src/idl_gen_lobster.cpp.o CMakeFiles/flatc.dir/src/idl_gen_lua.cpp.o CMakeFiles/flatc.dir/src/idl_gen_rust.cpp.o CMakeFiles/flatc.dir/src/idl_gen_fbs.cpp.o CMakeFiles/flatc.dir/src/idl_gen_grpc.cpp.o CMakeFiles/flatc.dir/src/idl_gen_json_schema.cpp.o CMakeFiles/flatc.dir/src/idl_gen_swift.cpp.o CMakeFiles/flatc.dir/src/flatc.cpp.o CMakeFiles/flatc.dir/src/flatc_main.cpp.o CMakeFiles/flatc.dir/src/code_generators.cpp.o CMakeFiles/flatc.dir/grpc/src/compiler/cpp_generator.cc.o CMakeFiles/flatc.dir/grpc/src/compiler/go_generator.cc.o CMakeFiles/flatc.dir/grpc/src/compiler/java_generator.cc.o CMakeFiles/flatc.dir/grpc/src/compiler/python_generator.cc.o CMakeFiles/flatc.dir/grpc/src/compiler/swift_generator.cc.o CMakeFiles/flatc.dir/grpc/src/compiler/ts_generator.cc.o -o flatc   && :

Copy link
Contributor

Choose a reason for hiding this comment

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

My guess is that the autotools are smart enough

no its not, as i recently discovered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have a build log that verifies it?
I think my build log linkec above, verifies that it is already solved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I cannot open the posted links even though I have created a discord account. Can you copy the plain text? Which autotools port id affected?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which auto tool port is affected? If have tested some and all is working fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

the discord discussion is about libpq but only if the CMAKE_C_COMPILER is correctly set as CC. currently it defaults to gcc instead of correctly using clang on osx. (see #22254)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless you do not install the real GNU complier from homebrew or such you have only clang.
Apple provides a gcc and g++ binaries that are only wrappers forwarding the command line to clang.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 101 to 105
foreach(arch IN LISTS CMAKE_OSX_ARCHITECTURES)
string(APPEND ${flag_var} " -arch ${arch}")
endforeach()
string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}")
endif()
string(APPEND ${flag_var} " -isysroot ${CMAKE_OSX_SYSROOT}")
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks like cmake should already be adding the flags (answer to https://github.com/microsoft/vcpkg/pull/21772/files#r760911069 ?)

Copy link
Contributor Author

@daschuer daschuer Dec 2, 2021

Choose a reason for hiding this comment

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

Unfortunately not. It adds the falgs only to the compiler variable but not to the linker. I am afraid we cannot add it to the linker here, because it might break autotools.

Copy link
Contributor

Choose a reason for hiding this comment

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

might break autotools.

That is not good enough. It either breaks or it doesn't. And if it breaks the question is: 'why?'

@JackBoosY
Copy link
Contributor

Can you please resolve the file confict?

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!

PRs must add only one version and must not modify any published versions

When making any changes to a library, the version or port-version in vcpkg.json or CONTROL must be modified.

Error: Local changes detected for fontconfig but no changes to version or port version.
-- Version: 2.13.94#4
-- Old SHA: 41142efe19b6591f0de47857a32fecdb8bbfc717
-- New SHA: eba73bda28f1b3c3cc3b3ee695f67b783b54abfc
-- Did you remember to update the version or port version?
-- Pass `--overwrite-version` to bypass this check.
***No files were updated.***

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 63e935d967a3410e26bf4a708efa39d8384d2bbb -- versions
./vcpkg x-add-version --all
Diff
diff --git a/versions/f-/fontconfig.json b/versions/f-/fontconfig.json
index 26d12f8..c5d5a8d 100644
--- a/versions/f-/fontconfig.json
+++ b/versions/f-/fontconfig.json
@@ -1,7 +1,7 @@
 {
   "versions": [
     {
-      "git-tree": "eba73bda28f1b3c3cc3b3ee695f67b783b54abfc",
+      "git-tree": "2f32046747209c234e60dc297b48d5bdc7ee4494",
       "version": "2.13.94",
       "port-version": 5
     },

cmake appends the -mmacosx-version-min flag c/cxx flags, overriding the
value set via VCPKG_C_FLAGS. By setting the environment variable, the
VCPKG_C_FLAGS value was used for meson builds. Now the same value is
taken for both.
@daschuer
Copy link
Contributor Author

daschuer commented Jan 15, 2022

The last commit fixes an issue that a different OSX_DEPLOYMENT_TARGET was used in case it was set via VCPKG_C_FLAGS. It would also be confusing it the VCPKG_DETECTED_CMAKE_C_FLAGS variable does not match the equivalent of cmake ports.

…C_FLAGS

This is done in the same way in CMake internally
@daschuer
Copy link
Contributor Author

What kind of responds do you expect? I have explained the pending topic here:
#21772 (comment)

@JackBoosY
Copy link
Contributor

What kind of responds do you expect? I have explained the pending topic here: #21772 (comment)

I think we should talk with @Neumann-A since I have less knowledge with meson.
But all configuration tools should use the same amount/content of configuration, why does meson need extra configuration here?

@daschuer
Copy link
Contributor Author

why does meson need extra configuration here?

cmake and autotools are taking -arch and -isysroot from the c flags and pass them to the linker. Meson does not do it.
The cmake behavior is the reason they are not in the linker flags when fetching DETECTED variables form cmake. The autotools behaviour is the reason that we can't manual add them to the fetched cmake variables, because otherwise they are duplicated within autotool builds.

Copy link
Contributor

@ras0219-msft ras0219-msft left a comment

Choose a reason for hiding this comment

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

Overall, this LGTM with one remaining question:

[adding the flags also to LDFLAGS results in] -arch and -isysroot applied twice.

Is this a problem? If not, I think it would be better to add them to both to avoid meson-specific changes. Or to look at it another way: to avoid autotools-specific omissions in the core helper.

Even if it is a problem, it almost seems like it would be more correct to remove them after the fact in the autotools helper since this is specifically an autotools issue as @Neumann-A pointed out. However I think it would be reasonable to merge this PR as-is and make that larger shuffle later (in this case).

@JackBoosY JackBoosY added requires:author-response and removed requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. labels Feb 5, 2022
@JackBoosY
Copy link
Contributor

Ping @daschuer

@daschuer
Copy link
Contributor Author

daschuer commented Feb 6, 2022

However I think it would be reasonable to merge this PR as-is and make that larger shuffle later (in this case).

Let's go with this. And keep the scope of this PR to meson.

@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Feb 23, 2022
@vicroms vicroms merged commit a34997a into microsoft:master Feb 24, 2022
ekilmer added a commit to ekilmer/vcpkg that referenced this pull request Feb 27, 2022
* master: (57 commits)
  [vcpkg-tools] update cmake and git (windows only) (microsoft#22985)
  Update vcpkg tool to 2022-02-24. (microsoft#23162)
  [vcpkg baseline] Move cspice headers (microsoft#23272)
  Fixed inaccurate Chinese words (microsoft#23179)
  [vcpkg] Add fixed changelog generator. (microsoft#23255)
  [authentication.md] Add Jenkins section (microsoft#23226)
  [vcpkg] Meson osx sysroot (microsoft#21772)
  [pkgconf] enable search for system libs on linux (microsoft#23010)
  [yasm/yasm-tool] Incorporate yasm-tool into yasm (microsoft#23218)
  [lapack-reference] Update to 3.10 (microsoft#23228)
  [skia] Arm64 for skia on osx (microsoft#23222)
  [libfido2] Update to 1.10.0 (microsoft#23241)
  [Tracy] Fixing issue where version 0.7.8 was pulling the wrong version (microsoft#23061)
  [libgpiod] Add new port. (microsoft#23221)
  [drogon] Update to 1.7.5 (microsoft#23227)
  [tinyexif] Remove from fail list. (microsoft#23163)
  [vcpkg docs][ES] Sync with English readme (microsoft#19834) (microsoft#22618)
  [vcpkg baseline][libao] Disable dlfcn check under windows (microsoft#23235)
  [OpenCV] upgrade to v4.5.5 (microsoft#22801)
  [libcurl-simple-https] New port (microsoft#22917)
  ...
@daschuer daschuer deleted the meson_osx_sysroot branch January 5, 2023 07:32
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:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants