Skip to content

[vcpkg osx] Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params#12657

Closed
Deadpikle wants to merge 7 commits intomicrosoft:masterfrom
Deadpikle:fix/osx-archs-attempt
Closed

[vcpkg osx] Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params#12657
Deadpikle wants to merge 7 commits intomicrosoft:masterfrom
Deadpikle:fix/osx-archs-attempt

Conversation

@Deadpikle
Copy link
Copy Markdown
Contributor

I discovered upon some tests with Xcode 12 that the arm64-osx triplet was not correct and didn't actually build for arm64 based on the output of lipo -archs mylib.a. After a long conversation in Discord, we discovered a few issues and made a few fixes:

  1. -DCMAKE_OSX_ARCHITECTURES should be set/used in order to properly tell cmake which architectures to build for. Based on the conversation in Discord, it was decided to set CMAKE_OSX_ARCHITECTURES based on the value of VCPKG_TARGET_ARCHITECTURE if the target was iOS or macOS.
  2. If you set multiple architectures, the args weren't escaped properly. That escaping is also fixed, here.
  3. In order to make the code more readable for checking the target, a VCPKG_TARGET_IS_IOS definition was added.
  4. To test out the multiple VCPKG_TARGET_ARCHITECTURE for arm64 with x86_64, a new community triplet was added.

In theory, now you can run commands like ./vcpkg install libzip[core] --triplet x64_arm64-osx and get a build that, with a lipo -archs libzip.a check, will contain both x86_64 and arm64 code.

CI:

No CI changes have been made.

Special thanks

Many thanks to @strega-nil and @ras0219-msft for their large amount of help in figuring things out in Discord. Thank you!

Deadpikle and others added 6 commits July 30, 2020 12:42
Use VCPKG_TARGET_ARCHITECTURE for CMAKE_OSX_ARCHITECTURES as well
This fixes osx triplets that want to build for an arch other than x86_64
@strega-nil strega-nil changed the title WIP: Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params [vcpkg osx] Fix building for non-x86_64 archs on macOS (OSX) and fixes escapes for some params Jul 30, 2020
@@ -0,0 +1,5 @@
set(VCPKG_TARGET_ARCHITECTURE x86_64 arm64)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be:

Suggested change
set(VCPKG_TARGET_ARCHITECTURE x86_64 arm64)
set(VCPKG_TARGET_ARCHITECTURE x64 arm64)

endif()

# make sure to escape multiple architectures in VCPKG_TARGET_ARCHITECTURE
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

And then here:

Suggested change
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
string(REPLACE ";" "\\;" _VCPKG_TARGET_ARCHITECTURE "${VCPKG_TARGET_ARCHITECTURE}")
string(REPLACE "x64" "x86_64" _VCPKG_TARGET_ARCHITECTURE "${_VCPKG_TARGET_ARCHITECTURE}")

thus, we actually write the vcpkg names for these targets, as opposed to the cmake names. I'm not sure if this is the right way, but it makes sense to me... cc @ras0219-msft ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The reason I think this is that in x86-windows, we set VCPKG_TARGET_ARCHITECTURE to x86, not Win32.

Copy link
Copy Markdown
Contributor

@strega-nil strega-nil Jul 30, 2020

Choose a reason for hiding this comment

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

Also, this should break x64-osx, so these suggestions will fix it; otherwise, we'll have to change x64-osx.

"-DVCPKG_CRT_LINKAGE=${VCPKG_CRT_LINKAGE}"
"-DVCPKG_LINKER_FLAGS=${VCPKG_LINKER_FLAGS}"
"-DVCPKG_TARGET_ARCHITECTURE=${VCPKG_TARGET_ARCHITECTURE}"
"-DVCPKG_TARGET_ARCHITECTURE=${_VCPKG_TARGET_ARCHITECTURE}"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should this be passed only if !TARGET_IS_OSX?

If you changed VCPKG_TARGET_ARCHITECTURE to use x86_64, prettify failed with the error .
@ras0219
Copy link
Copy Markdown
Contributor

ras0219 commented Jul 30, 2020

After reviewing this code and examining the various uses and cases for VCPKG_TARGET_ARCHITECTURE throughout the code[1], I think that there are some deeper issues that will eventually need to be addressed.

For example, how do we handle the many existing libraries that check the target architecture to add target-specific code (such as assembly files)? This is an issue in CMake as much as it is in other buildsystems. I don't think it is realistic to assert that every buildsystem and every library will be patchable to support lipo'd binaries -- and even a single link in the chain breaks the entire system.

There are other obvious issues, such as where do you stuff flags specifically for one or the other target? Maybe CMake has generator expressions, but again, what about autoconf/make/bazel/meson/etc?

Until we have a credible solution for these issues, I don't see how vcpkg can properly support universal binaries. I suspect a solution will at a minimum require vcpkg to invoke separate underlying builds for each architecture (similar to how we have separate release and debug builds), and then owning the lipo process to merge the output binaries -- however even this may be insufficient due to unknown obstacles. Depending on the use case, maybe this is more appropriate as a custom exporter that at export time lipos together multiple triplets into an SDK.

Therefore, because any short-term solution is going to be a hack, I think that the design here should adjust away from how to properly support it (since I do not think we have answers to all the above questions at this time) and instead focus on what exactly @Deadpikle is looking to achieve in their specific situation. Because we already support VCPKG_OSX_ARCHITECTURES, it seems appropriate to just fix the handling of lists for this particular item and have @Deadpikle create a custom triplet that sets it to x86_64;arm64. I do not think it's appropriate to create a community triplet for this at this time because it's going to fall over as soon as it sees a moderately complicated CMake project or anything besides CMake.

tl;dr:

  1. I think this is a much more difficult problem than initially expected.
  2. Therefore I think we should stick with the existing VCPKG_OSX_ARCHITECTURES (and fix the bugs) instead of trying to make things seamlessly work with VCPKG_TARGET_ARCHITECTURE
  3. I think things here are too experimental and unstable for even a community triplet
  4. You probably will want to set VCPKG_TARGET_ARCHITECTURE to x64 instead of ignore to maximize the chances for things to "just work", but many libraries will likely still be pretty broken.

[1] git grep "VCPKG_TARGET_ARCHITECTURE"

@Deadpikle
Copy link
Copy Markdown
Contributor Author

Thanks for the feedback on the PR and for taking a deeper look.

I think it's safe to say that the PR could probably be closed/shelved and then the following action items taken:

  1. Submit a PR that fixes the arm64-osx triplet to use VCPKG_OSX_ARCHITECTURES. I don't see why that wouldn't work pretty much out of the box.
  2. Submit a second PR for the lists in configuration vars (this would allow me to create a custom triplet with two VCPKG_OSX_ARCHITECTURES archs, as you noted) -- again, this was entirely @strega-nil, here, so I feel a little bit odd acting like it was me, even if the commits are cherry-picked from her work. Credit where credit is due and all that! 😄
  3. Submit a 3rd PR with the prettify fixes (or let @strega-nil do that since it was entirely her fix and there won't be an open PR that exposes that problem?)

This would keep things fairly separated for now and avoid getting into the deeper issues you've noted. Note: I can do without items 2 and 3 for my use case -- I can manually set up my dependency building script to lipo things for me. Really, the main "broken" thing here was that the arm64-osx triplet didn't work at all (and, frankly, I suppose I could just manually edit the existing triplet, anyway.)

@strega-nil
Copy link
Copy Markdown
Contributor

strega-nil commented Jul 30, 2020

The main concern for me is that it's really important that this gets done before the full ARM macOS devices are released. We need to make sure that we get some engineering time on this.

@ras0219
Copy link
Copy Markdown
Contributor

ras0219 commented Jul 30, 2020

Really, the main "broken" thing here was that the arm64-osx triplet didn't work at all

So, focusing on that specifically, how do we get that fixed? Does it just need to have set(VCPKG_OSX_ARCHITECTURES arm64) added?

I would really like to have the list fixes; those are just goodness. I think it would be ideal to open a new PR with just those changes (list fixes + whatever is needed to fix arm64-osx without the lipo rabbit hole).

@Deadpikle
Copy link
Copy Markdown
Contributor Author

So, focusing on that specifically, how do we get that fixed? Does it just need to have set(VCPKG_OSX_ARCHITECTURES arm64) added?

I believe that's what was needed, yes. I will take a look at this again on Monday and verify that unless something else comes up and will submit new PR(s) as needed to keep things clean.

Deadpikle added a commit to Deadpikle/vcpkg that referenced this pull request Aug 3, 2020
If you changed VCPKG_TARGET_ARCHITECTURE to use x86_64, the prettify command failed to parse things properly. This was found during the changes made for microsoft#12657 and was completed by @strega-nil (thanks!).
Deadpikle added a commit to Deadpikle/vcpkg that referenced this pull request Aug 3, 2020
e.g., if you set multiple architectures for VCPKG_OSX_ARCHITECTURES, the arguments were not escaped properly. This PR fixes that.
The code from this PR was pulled from microsoft#12657 and was done entirely by @strega-nil.
@Deadpikle
Copy link
Copy Markdown
Contributor Author

So, focusing on that specifically, how do we get that fixed? Does it just need to have set(VCPKG_OSX_ARCHITECTURES arm64) added?

I believe that's what was needed, yes. I will take a look at this again on Monday and verify that unless something else comes up and will submit new PR(s) as needed to keep things clean.

OK, yes, all that was needed to fix the arm64-osx build was VCPKG_OSX_ARCHITECTURES. So, I have split this PR into multiple PRs for easier review and to have more focus:

  1. [vcpkg] Add VCPKG_TARGET_IS_IOS #12715 -- adding the VCPKG_TARGET_IS_IOS common definition.
  2. [vcpkg] Fix arm64-osx triplet not building for arm64 #12716 -- the actual fix for the arm64-osx community triplet
  3. [vcpkg] Fix prettify output not working in some cases #12717 -- fixing the prettify output
  4. Fix escaping of list items from triplets #12718 -- list fixes

With those PRs, I should be able to do everything I want to do with my own triplets or whatnot. I see that there's some discussion above, so I'm hesitant to close this PR directly until that discussion is done.

Really appreciate all who have been involved thus far. :)

@PhoebeHui PhoebeHui self-assigned this Aug 4, 2020
@PhoebeHui PhoebeHui added the category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed label Aug 4, 2020
strega-nil pushed a commit that referenced this pull request Aug 5, 2020
If you changed VCPKG_TARGET_ARCHITECTURE to use x86_64, the prettify command failed to parse things properly. This was found during the changes made for #12657 and was completed by @strega-nil (thanks!).
hellozee pushed a commit to hellozee/vcpkg that referenced this pull request Sep 11, 2020
If you changed VCPKG_TARGET_ARCHITECTURE to use x86_64, the prettify command failed to parse things properly. This was found during the changes made for microsoft#12657 and was completed by @strega-nil (thanks!).
@JackBoosY JackBoosY added the depends:different-pr This PR or Issue depends on a PR which has been filed label Oct 29, 2020
@strega-nil
Copy link
Copy Markdown
Contributor

Closing since we have other PRs merged that do the same thing; thanks so much @Deadpikle for your contributions!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category:vcpkg-feature The issue is a new capability of the tool that doesn’t already exist and we haven’t committed depends:different-pr This PR or Issue depends on a PR which has been filed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants