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

[icu] Add support for macos rpath prefix macro #15706

Merged
merged 4 commits into from
Jan 26, 2021

Conversation

adam-bloom
Copy link
Contributor

Describe the pull request

  • What does your PR fix?
    This PR implements a temporary solution for https://unicode-org.atlassian.net/browse/ICU-21458. It alters the ID fields of the built dylibs to add support for the @rpath macro on macOS. The new code is commented that it should be removed and replaced with calls to new configure options, if ICU-21458 is resolved.

  • Which triplets are supported/not supported? Have you updated the CI baseline?
    Only macOS is affected.

  • Does your PR follow the maintainer guide?
    Yes

@adam-bloom
Copy link
Contributor Author

The macOS end-to-end tests are failing due to:

Failed to take the filesystem lock on /Volumes/data/work/1/s/.vcpkg-root:
    Resource busy

I don't believe this has anything to do with the changes in this PR, since this PR does not touch core vcpkg itself. Any ideas?

@JackBoosY JackBoosY self-assigned this Jan 18, 2021
@JackBoosY JackBoosY added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Jan 18, 2021
ports/icu/portfile.cmake Outdated Show resolved Hide resolved
ports/icu/portfile.cmake Outdated Show resolved Hide resolved
@JackBoosY
Copy link
Contributor

I'd like @strega-nil to review this PR also.

@strega-nil
Copy link
Contributor

@adam-bloom This is unfortunately a change we cannot accept; changing the output depending on whether a tool is found is unacceptable for us. How common is install_name_tool? When does it exist on the system? We can discuss better solutions depending on that.

@adam-bloom
Copy link
Contributor Author

@adam-bloom This is unfortunately a change we cannot accept; changing the output depending on whether a tool is found is unacceptable for us. How common is install_name_tool? When does it exist on the system? We can discuss better solutions depending on that.

install_name_tool is in /usr/bin on macOS, which is a part of the write restricted system partition. I don't have access to older macOS installations, so I'm not sure when it was first included. The man page has a date of 2009 there - so likely 10.6 was the first inclusion. Additionally, Apple Developer Tools is listed as a vcpkg requirement in the readme, and another copy of install_name_tool is included with Xcode command line tools: /Library/Developer/CommandLineTools/usr/bin/install_name_tool.

In other words, I think every macOS system running vcpkg should be compatible with this. If, however, someone attempted to build an osx triplet from another OS (I'm not sure if that would even work without the macOS SDK...), they might be missing this tool and the step would skip.

I'll note that I initially did not have the if present check, since I assumed all osx vcpkg triplet runs would be from macOS systems that have this tool. I added that at the request of @JackBoosY

@strega-nil
Copy link
Contributor

I agree with using find_program, but it should be REQUIRED, given what you're saying :)

ports/icu/portfile.cmake Outdated Show resolved Hide resolved
@adam-bloom
Copy link
Contributor Author

Is there a use case for vcpkg where a osx triplet is built on a non-macOS system? Or is this definitely safe to mark as required?

@JackBoosY
Copy link
Contributor

@strega-nil The judgment is if(VCPKG_TARGET_IS_OSX AND VCPKG_LIBRARY_LINKAGE STREQUAL "dynamic"), I think it's fine.
Do you have any other questions?

@adam-bloom Nope.

@strega-nil
Copy link
Contributor

yeah, the issue I have is making it non-REQUIRED; since we'll always be building on macOS, we should REQUIRED the program.

@JackBoosY JackBoosY added info:reviewed Pull Request changes follow basic guidelines and removed requires:discussion labels Jan 21, 2021
@JackBoosY
Copy link
Contributor

So I think it's good now.

@JackBoosY JackBoosY removed the info:reviewed Pull Request changes follow basic guidelines label Jan 22, 2021
@JackBoosY JackBoosY added the info:reviewed Pull Request changes follow basic guidelines label Jan 25, 2021
@JackBoosY
Copy link
Contributor

Already merged from master.

@dan-shaw dan-shaw merged commit e72302d into microsoft:master Jan 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants