Skip to content

Comments

alembic: Fix install destinations#215528

Merged
davidak merged 3 commits intoNixOS:masterfrom
hesiod:alembic
Mar 10, 2023
Merged

alembic: Fix install destinations#215528
davidak merged 3 commits intoNixOS:masterfrom
hesiod:alembic

Conversation

@hesiod
Copy link
Contributor

@hesiod hesiod commented Feb 9, 2023 •
edited
Loading

Note: I've written a tester (testImportedCMakePackage) in order to prevent issues like the one described below in the future. It's written in a generic fashion and should be applicable to other CMake projects as well. It currently resides below alembic's directory, in pkgs/development/libraries/alembic/cmakePackageTests, as I'm not sure about the interest in such a tester. If there's interest in using it as a generic tester I could also move it to pkgs/build-support/testers/. It is somewhat comparable to the hasPkgConfigModule tester that is already located there.
Edit: I've moved the tester to a separate commit for now in order to streamline this PR a bit.

Description of changes

The generated CMake targets file was referring to an incorrect destination as the derivation manually moved the libraries during installPhase, while CMake uses the path it thinks is going to be used (the DESTINATION in the install rule) in the IMPORTED_LOCATION property.

By setting the install destinations via CMake flags (and patching the DESTINATION for the binary install rules), CMake will pick up the correct locations in the generated AlembicTargets-release.cmake file.

Along with fixing that issue, this commit also includes the following changes:

  • Remove unused unzip nativeBuildInput
  • Add missing direct dependency imath: Previously it was only picked up indirectly, resulting in CMake configuration warnings
  • Add imath ilmbase as propagatedBuildInput: Downstream users of Alembic (via CMake) need to add imath ilmbase as a dependency as well. For some reason this is not discovered correctly otherwise
  • Use CMake setup hooks instead of setting buildPhase/installPhase
  • Add maintainer tmarkus
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.05 Release Notes (or backporting 22.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from guibou February 9, 2023 16:41
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Feb 9, 2023
@hesiod hesiod marked this pull request as draft February 9, 2023 17:42
@hesiod hesiod force-pushed the alembic branch 5 times, most recently from 6213e65 to ca23df9 Compare February 10, 2023 20:00
@hesiod hesiod marked this pull request as ready for review February 10, 2023 20:39
@hesiod hesiod mentioned this pull request Feb 10, 2023
Merged
12 tasks
hesiod added a commit to hesiod/nixpkgs that referenced this pull request Feb 10, 2023
@hesiod
pyside2: Add qt3d to buildInputs
53cd64c 
PySide2 will not build its qt3d component unless qt3d is added to
buildInputs.
This is part of the effort to package Meshroom.
(Related Meshroom PRs: NixOS#215528 and NixOS#215728)
@ofborg ofborg bot requested review from cillianderoiste and veprbl February 10, 2023 22:38
@hesiod hesiod force-pushed the alembic branch 2 times, most recently from 82ad32c to 29bbc9c Compare February 11, 2023 01:02
@davidak
Copy link
Member

davidak commented Feb 14, 2023

Can you add the test (testImportedCMakePackage) in a separate commit? The file can later be moved if there is interest.

@hesiod
Copy link
Contributor Author

hesiod commented Feb 14, 2023

@davidak

Can you add the test (testImportedCMakePackage) in a separate commit? The file can later be moved if there is interest.

Yeah, good point, I'll do that.

If you're reviewing this PR atm: Please note that there's a minor correction I've made locally I haven't pushed yet: propagatedBuildOutputs needs to contain "lib". The full line should be propagatedBuildOutputs = [ "lib" ];.
Apparently I forgot to push that commit earlier. I won't force push right now so that the current check results remain visible.

@davidak
Copy link
Member

davidak commented Feb 14, 2023

I tried to review and i haven't found an issue, but i'm not comfortable to review the cmake test, so i leave it to someone else.

Thanks for your efforts!

@hesiod
Copy link
Contributor Author

hesiod commented Feb 14, 2023

Marking this PR as a draft while I reorganize and improve it a bit

@hesiod hesiod marked this pull request as draft February 14, 2023 23:39
This was referenced Feb 14, 2023
Merged
Draft
Merged
Draft
@hesiod hesiod requested a review from davidak February 17, 2023 13:38
@hesiod hesiod marked this pull request as ready for review February 17, 2023 13:38
davidak
davidak reviewed Feb 17, 2023
View reviewed changes
@davidak
Copy link
Member

davidak commented Feb 17, 2023 •
edited
Loading

Result of nixpkgs-review pr 215528 run on x86_64-linux 1

3 packages failed to build:
  • blender
  • blender-hip
  • python310Packages.bpycv
1 package built:
  • alembic

The errors look unrelated:

error: builder for '/nix/store/2ay0ynwd7639msidi0f8xjbx518l7k4m-openimagedenoise-1.4.3.drv' failed with exit code 1;
       last 10 log lines:
       > -- Found TBB version 2020.3 at /nix/store/6y5ms2chn50ksw6fbsig8jm62fimqspr-tbb-2020.3-dev
       > CMake Error at cmake/FindTBB.cmake:80 (message):
       >   Cannot find required components: ;tbb
       > Call Stack (most recent call first):
       >   cmake/FindTBB.cmake:487 (rk_tbb_error)
       >   CMakeLists.txt:73 (find_package)
       >
       > 
       > -- Configuring incomplete, errors occurred!
       > See also "/build/source/build/CMakeFiles/CMakeOutput.log".
       For full logs, run 'nix log /nix/store/2ay0ynwd7639msidi0f8xjbx518l7k4m-openimagedenoise-1.4.3.drv'.
error: builder for '/nix/store/a8bdg1wpj54fs7wafbn6n9k90gc2kxfr-embree-3.13.5.drv' failed with exit code 1;
       last 10 log lines:
       > -- Found TBB version 2020.3 at /nix/store/6y5ms2chn50ksw6fbsig8jm62fimqspr-tbb-2020.3-dev
       > CMake Error at common/cmake/FindTBB.cmake:72 (message):
       >   Cannot find required components: ;tbb
       > Call Stack (most recent call first):
       >   common/cmake/FindTBB.cmake:477 (rk_tbb_error)
       >   common/tasking/CMakeLists.txt:30 (find_package)
       >
       > 
       > -- Configuring incomplete, errors occurred!
       > See also "/build/source/build/CMakeFiles/CMakeOutput.log".
       For full logs, run 'nix log /nix/store/a8bdg1wpj54fs7wafbn6n9k90gc2kxfr-embree-3.13.5.drv'.
error: 2 dependencies of derivation '/nix/store/hqz2iip8hb7v3l9jh0b94smdbiz77pkj-blender-3.3.1.drv' failed to build
error: 2 dependencies of derivation '/nix/store/m5zrcl5hy24kbxawwx4433v5g1pb78fn-blender-3.3.1.drv' failed to build
error: 1 dependencies of derivation '/nix/store/nw0nh38h0vg6lw13ra57hrdi6ql1khs3-python3.10-bpycv-0.2.43.drv' failed to build

@hesiod
Copy link
Contributor Author

hesiod commented Feb 17, 2023

@davidak Thanks, I think those failures are fallout due to #212787 (more precisely, commit f386825).
See the following Hydra failures:

I was going to create an issue, but it turns out issues already exist for those failures: #216580, #216390

hesiod added 3 commits March 9, 2023 23:59
@hesiod
alembic: Fix install destinations
79aebb6 
The generated CMake targets file was referring to an incorrect
destination as the derivation manually moved the libraries during
installPhase, while CMake uses the path it thinks is going to be used
(the DESTINATION in the install rule) in the IMPORTED_LOCATION property.

By setting the install destinations via CMake flags (and patching the
DESTINATION for the binary install rules), CMake will pick up the
correct locations in the generated AlembicTargets-release.cmake file.

Along with fixing that issue, this commit also includes the following
changes:
* Remove unused unzip nativeBuildInput
* Enable unit tests
* Add missing direct dependency ilmbase:
  Previously it was only picked up indirectly, resulting in CMake
  configuration warnings
* Add ilmbase as propagatedBuildInput:
  Downstream users of Alembic (via CMake) need to add ilmbase as a
  dependency as well
  For some reason this is not discovered correctly otherwise
* Use CMake setup hooks instead of setting buildPhase/installPhase
@hesiod
alembic: Add maintainer tmarkus
54e1189
@hesiod
blender: Fix build after changes in Alembic package
edcd384
@hesiod hesiod marked this pull request as draft March 9, 2023 23:00
@hesiod hesiod marked this pull request as ready for review March 9, 2023 23:01
@davidak
Copy link
Member

davidak commented Mar 10, 2023

Result of nixpkgs-review pr 215528 run on x86_64-linux 1

8 packages built:
  • alembic
  • alembic.bin
  • alembic.dev
  • alembic.lib
  • blender
  • blender-hip
  • python310Packages.bpycv
  • python310Packages.bpycv.dist

davidak
davidak approved these changes Mar 10, 2023
View reviewed changes
Copy link
Member

@davidak davidak left a comment

Choose a reason for hiding this comment

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

Change looks good

Builds, works

@davidak davidak merged commit c233aa6 into NixOS:master Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guibou guibou Awaiting requested review from guibou

@cillianderoiste cillianderoiste Awaiting requested review from cillianderoiste

@veprbl veprbl Awaiting requested review from veprbl

1 more reviewer

@davidak davidak davidak approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@hesiod @davidak