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

fluidsynth: Fix CMake config #242657

Merged
merged 1 commit into from
Jul 10, 2023
Merged

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Jul 10, 2023

Description of changes

Closes #242623

Upstream is concatenating CMAKE_INSTALL_LIBDIR onto CMAKE_INSTALL_PREFIX to get a directory to use for the install_name on Darwin. This is not the right way of making CMAKE_INSTALL_LIBDIR absolute.

The config was first set up for breakage when the CMAKE_INSTALL_LIBDIR was made relative, in order to work around the wrong concatenation. This caused the _IMPORT_PREFIX to be computed by walking up the directory tree from the config file, instead of using the absolute CMAKE_INSTALL_PREFIX.

Then outputs were introduced to the derivation and the config was moved to a different output. Now the prefix it walks up to is the wrong prefix, and the config is busted.

Fix by reverting the original workaround so we have an absolute CMAKE_INSTALL_LIBDIR, and pulling a patch that fixes the bad concatenation.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@marsam
Copy link
Contributor

marsam commented Jul 10, 2023

Would this supersede #242635?

@OPNA2608
Copy link
Contributor Author

Would this supersede #242635?

Yes.

Upstream is concatenating CMAKE_INSTALL_LIBDIR onto CMAKE_INSTALL_PREFIX to get
a directory to use for the install_name on Darwin. This is not the right way of
making CMAKE_INSTALL_LIBDIR absolute.

The config was first set up for breakage when the CMAKE_INSTALL_LIBDIR was made relative,
in order to work around the wrong concatenation. This caused the _IMPORT_PREFIX to be
computed by walking up the directory tree from the config file, instead of using
the absolute CMAKE_INSTALL_LIBDIR.

Then outputs were introduced to the derivation and the config was moved to a different
output. Now the prefix it walks up to is the wrong prefix, and the config is busted.

Fix by reverting the original workaround so we have an absolute CMAKE_INSTALL_LIBDIR,
and pulling a patch that fixes the bad concatenation.
@OPNA2608 OPNA2608 force-pushed the fix/fluidsynth_cmake_config branch from f427432 to f43a46f Compare July 10, 2023 15:14
@OPNA2608
Copy link
Contributor Author

  • FluidSynthTargets.cmake is back to using an absolute path of fluidsynth.out.
    set(_IMPORT_PREFIX "/nix/store/wz4zq40h5hymr1phfdwg64xliwai7xy0-fluidsynth-2.3.2")

  • install_name on Darwin looks good

$ otool -L /nix/store/am19r6l5d1p1dx1955sahd9dncwhq9lk-fluidsynth-2.3.2/lib/libfluidsynth.dylib | head -n2
/nix/store/am19r6l5d1p1dx1955sahd9dncwhq9lk-fluidsynth-2.3.2/lib/libfluidsynth.dylib:
        /nix/store/am19r6l5d1p1dx1955sahd9dncwhq9lk-fluidsynth-2.3.2/lib/libfluidsynth.3.dylib (compatibility version 3.0.0, current version 3.2.0)
  • building easyrpg-player with fluidsynth works now

Copy link
Contributor

@trofi trofi left a comment

Choose a reason for hiding this comment

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

Thank you!

@trofi trofi merged commit 4701d81 into NixOS:staging Jul 10, 2023
12 of 14 checks passed
@github-actions
Copy link
Contributor

Successfully created backport PR for staging-23.05:

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

Successfully merging this pull request may close these issues.

3 participants