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

Fix macOS icu dylib loading problems when MacPorts is not installed #1346

Merged
merged 1 commit into from
Nov 25, 2024

Conversation

lyonsil
Copy link
Member

@lyonsil lyonsil commented Nov 25, 2024

To decipher the changes made:

  1. We need to add +universal to the MacPorts install of icu libs or we only get libs matching the architecture of the build machine (i.e., x64 or arm64). That would leave one architecture always broken, and it would vary which is broken depending on whether an arm64 or x64 machine was used for building. Universal libraries are basically containers that contain 2 libraries, one for x64 and one for arm64. Our build process doesn't provide a simple way to build separately for different architectures, so including universal libraries is a simple fix even though it makes the download larger since you'll always include code that isn't for your architecture. Because I'm not sure if every build of ICU on MacPorts will include universal libs, I locked in the version number to one that does.
  2. We need to fix up the loader paths embedded inside each dylib so it doesn't look for its dependent dylibs only in the path where they were on the machine used to build the ICU dylib files originally that we download from MacPorts. As it stands now, when you load one of the ICU dylibs, it always looks for its dependencies under /opt/local/lib. Unless the machine running Platform.Bible has ICU installed from MacPorts (which puts them under /opt/local/lib), then loading ICU dylibs will not work in P.B. Apple provides a tool (install_name_tool) that can rewrite locations inside of a library where to look for each dependency.
  3. I updated the .NET copying of ICU libs to exclude two that are only meant for testing. They aren't needed in production, and we don't need them for our own testing, either. icu.net might use them, but P.B does not.
  4. I updated the version of icu.net to one that includes trace logging if loading ICU libraries fails. I updated the .NET program to log that extra trace information to the console when running on macOS. I didn't add it for other OSes for now. We can always adjust that later easily.

This change is Reviewable

@lyonsil lyonsil linked an issue Nov 25, 2024 that may be closed by this pull request
Copy link
Contributor

@irahopkinson irahopkinson left a comment

Choose a reason for hiding this comment

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

:lgtm: Thanks for figuring out all these details for macOS - great work!

For 1 we can change our GHA matrix to something like this to build on both macOS architectures:

strategy:
  matrix:
    os: [windows-latest, macos-latest, ubuntu-latest]
    dotnet_version: [8.0.x]
    include:
      - os: macos-latest
        arch: [x64, arm64]
    exclude:
      - os: windows-latest
        arch: arm64
      - os: ubuntu-latest 
        arch: arm64

You could elect to get this PR in as is and set this as a follow-up task.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member Author

@lyonsil lyonsil left a comment

Choose a reason for hiding this comment

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

We're actually already building for both architectures on macOS. If you look at the build artifacts both exist even though currently they aren't built correct as far as ICU is concerned. Maybe it's only an issue with MacPorts (i.e., I don't see any way to tell their port tool to download libs for any architecture other than the one that is running now), but I don't see any way for us to specify with our own builds that I want to produce artifacts for one specific architecture. This might be buried deeper within the ERB scripts, but it seems like we don't have a clean way to cross-compile. That's probably why we have to have each OS build artifacts for itself instead of having a single OS and architecture build artifacts for all 3 OSes.

All this being said, I'll create a low priority ticket if someone wants to figure this out later. The bloat from universal ICU libs is not that significant since there are only 4 ICU dylib files. I'm sure bloat from other things is larger. For example, I'm pretty sure just including the .NET runtime is the biggest contributor because we're including so many framework DLLs.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@lyonsil lyonsil merged commit 03846fc into main Nov 25, 2024
7 checks passed
@lyonsil lyonsil deleted the 1177-macos-library-loading branch November 25, 2024 20:42
Copy link
Member

@tjcouch-sil tjcouch-sil left a comment

Choose a reason for hiding this comment

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

🫨😲unbelievable! Thanks, Matt!! 🎉🎉🎉

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Projects don't open properly on macOS
3 participants