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

Blender darwin #307833

Merged
merged 9 commits into from
May 13, 2024
Merged

Blender darwin #307833

merged 9 commits into from
May 13, 2024

Conversation

gador
Copy link
Member

@gador gador commented Apr 29, 2024

Description of changes

This PR fixes the build of blender on darwin.

To do so, the following changes have been made:

  • add sse2neon package (fixed compiler warning)
  • fixed openpgl darwin build (needed as build input)
  • fixed openimagedenoise darwin build. This could theoretically build with metal M1 support, but due to the older apple sdk in nixpkgs, this doesn't work, yet. When the sdk is updated, changes should be easy to include metalsupport.
  • add materialx. This fixes another compiler warning, enables the materialx build flag and can be used in other packages, too
  • add NanoVDB libraries included by openvdb to also be built (needed by blender)
  • python311Packages.openusd enable materialx build flag

Due to these changes, blender can now be build with sse2neon, materialx and open/nanoVDB support.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@afh
Copy link
Member

afh commented Apr 29, 2024

@gador I applaud the effort 👏

There is a draft PR out that updates openpgl from 0.5.0 to 0.6.0 (and removes superfluous use of pname); maybe those changes are useful for your work on this here?

Copy link
Member

@amarshall amarshall left a comment

Choose a reason for hiding this comment

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

I stopped commenting the final commit at some point because there were too many seemingly-unrelated unexplained changes. Please make atomic commits.

pkgs/by-name/ss/sse2neon/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sse2neon/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sse2neon/package.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/openimagedenoise/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
@gador
Copy link
Member Author

gador commented Apr 30, 2024

@gador I applaud the effort 👏

There is a draft PR out that updates openpgl from 0.5.0 to 0.6.0 (and removes superfluous use of pname); maybe those changes are useful for your work on this here?

I'll have a look at it!

@gador
Copy link
Member Author

gador commented Apr 30, 2024

I stopped commenting the final commit at some point because there were too many seemingly-unrelated unexplained changes. Please make atomic commits.

First: Thank you for taking your time reviewing!

The changes do concern non-darwin, since (e.g. the inclusion of sse2neon or materialx) can be used in the non-darwin build, too. Ijust worked on this with both in mind and didn't see any reason to not include it in the non-darwin build, too.

But of course this should go into a seperate commit. I'll work through your comments and split the blender commit into seperate ones.

Copy link
Contributor

@ShaddyDC ShaddyDC left a comment

Choose a reason for hiding this comment

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

I'm not sure how to test the materialx support in openusd, but otherwise, beyond what was mentioned, the changes to that package look fine to me.

@gador
Copy link
Member Author

gador commented Apr 30, 2024

@ShaddyDC thanks!

I work on refactoring and splitting the unbreak-darwin and add-other-dependencies now. Unfortunatly testing and building takes quite some time.

@gador gador force-pushed the blender-darwin branch 2 times, most recently from 4af50b9 to 58569e7 Compare April 30, 2024 09:50
@ofborg ofborg bot requested review from ShaddyDC and amarshall April 30, 2024 10:17
@gador
Copy link
Member Author

gador commented Apr 30, 2024

Result of nixpkgs-review pr 307833 run on aarch64-darwin 1

1 package failed to build:
  • super-slicer
12 packages built:
  • blender
  • materialx
  • openimagedenoise
  • openpgl
  • openusd
  • openvdb
  • openvdb.dev
  • prusa-slicer
  • python311Packages.openusd
  • sse2neon
  • super-slicer-beta
  • super-slicer-latest

@afh
Copy link
Member

afh commented Apr 30, 2024

Result of nixpkgs-review pr 307833 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • blender-hip
10 packages built:
  • blender
  • materialx
  • openimagedenoise
  • openpgl
  • openusd
  • openvdb
  • openvdb.dev
  • prusa-slicer
  • python311Packages.openusd
  • sse2neon

@gador
Copy link
Member Author

gador commented Apr 30, 2024

materialx and blender currently fail for x86_64-darwin. It seems to be caused by the old sdk in nixpkgs. I'm looking into it

@gador
Copy link
Member Author

gador commented Apr 30, 2024

This is a semi-automatic executed nixpkgs-review with nixpkgs-review-checks extension. It is checked by a human on a best effort basis and does not build all packages (e.g. lumo, tensorflow or pytorch).

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

1 package failed to build and already failed to build on hydra master:
1 package failed to build and are new build failure:
19 packages built:
  • bambu-studio
  • bambu-studio.debug
  • blender
  • blender-hip
  • materialx
  • openimagedenoise
  • openusd
  • openvdb
  • openvdb.dev
  • orca-slicer
  • orca-slicer.debug
  • prusa-slicer
  • prusa-slicer.debug
  • python311Packages.openusd
  • sse2neon
  • super-slicer-beta
  • super-slicer-beta.debug
  • super-slicer-latest
  • super-slicer-latest.debug

The following issues got detected with the above build packages.
Please fix at least the ones listed with your changed packages:

bambu-studio.debug:

got build log for '/nix/store/r9dadl15hpk57dabwn7qj0sc1bzgxi12-bambu-studio-01.09.00.70-debug' from 'daemon'
When evaluating attribute ‘bambu-studio.debug’:
warning: unused-argument
Unused argument: qhull.
Near pkgs/applications/misc/bambu-studio/default.nix:40:3:

   |
40 |   qhull,
   |   ^

warning: unused-argument
Unused argument: fetchpatch.
Near pkgs/applications/misc/bambu-studio/default.nix:46:3:

   |
46 |   fetchpatch,
   |   ^
bambu-studio:

got build log for '/nix/store/b884lc5clrmjb9rpzkjks3dqharrjrjf-bambu-studio-01.09.00.70' from 'daemon'
When evaluating attribute ‘bambu-studio’:
warning: environment-variables-go-to-env
Environment variable NIX_CFLAGS_COMPILE should be moved to env attribute rather than being passed directly to ‘stdenv.mkDerivation’.

Near pkgs/applications/misc/bambu-studio/default.nix:140:3:

    |
140 |   NIX_CFLAGS_COMPILE = "-Wno-ignored-attributes";
    |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/environment-variables-go-to-env.md
warning: environment-variables-go-to-env
Environment variable NIX_LDFLAGS should be moved to env attribute rather than being passed directly to ‘stdenv.mkDerivation’.

Near pkgs/applications/misc/bambu-studio/default.nix:143:3:

    |
143 |   NIX_LDFLAGS = lib.optionalString withSystemd "-ludev";
    |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/environment-variables-go-to-env.md
warning: unused-argument
Unused argument: qhull.
Near pkgs/applications/misc/bambu-studio/default.nix:40:3:

   |
40 |   qhull,
   |   ^

warning: unused-argument
Unused argument: fetchpatch.
Near pkgs/applications/misc/bambu-studio/default.nix:46:3:

   |
46 |   fetchpatch,
   |   ^
blender-hip:

got build log for '/nix/store/r6g53af4f9yq0zsdbgywpajdd3d8ai39-blender-4.1.1' from 'daemon'
When evaluating attribute ‘blender-hip’:
warning: unused-argument
Unused argument: darwin.
Near pkgs/applications/misc/blender/default.nix:7:3:

  |
7 |   darwin,
  |   ^
blender:

got build log for '/nix/store/0caa3m6c18xi2m2iz6m9l0z0449z0hns-blender-4.1.1' from 'daemon'
When evaluating attribute ‘blender’:
warning: unused-argument
Unused argument: darwin.
Near pkgs/applications/misc/blender/default.nix:7:3:

  |
7 |   darwin,
  |   ^
prusa-slicer:

got build log for '/nix/store/2n41jq5ssm44rwz7275bxvcvr2xp9wy8-prusa-slicer-2.7.4' from 'daemon'
When evaluating attribute ‘prusa-slicer’:
warning: environment-variables-go-to-env
Environment variable NIX_LDFLAGS should be moved to env attribute rather than being passed directly to ‘stdenv.mkDerivation’.

Near pkgs/applications/misc/prusa-slicer/default.nix:128:3:

    |
128 |   NIX_LDFLAGS = lib.optionalString withSystemd "-ludev";
    |   ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/environment-variables-go-to-env.md

super-slicer-latest.debug:

got build log for '/nix/store/mny6a72bcynpsdx2hgpy6rjab2zjfs37-super-slicer-2.4.58.5-debug' from 'daemon'
When evaluating attribute ‘super-slicer-latest.debug’:
warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern list(APPEND\ wxWidgets_LIBRARIES\ libexpat) doesn't match anything in file 'src/CMakeLists.txt'
Near pkgs/applications/misc/prusa-slicer/super-slicer.nix:83:

   |
83 |       inherit description;
   | ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md
warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern libexpat doesn't match anything in file 'src/libslic3r/CMakeLists.txt'
Near pkgs/applications/misc/prusa-slicer/super-slicer.nix:83:

   |
83 |       inherit description;
   | ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md

super-slicer-latest:

got build log for '/nix/store/ddi8z5gfvqs1kjsg3d49i2fjbv07cxzv-super-slicer-2.4.58.5' from 'daemon'
When evaluating attribute ‘super-slicer-latest’:
warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern list(APPEND\ wxWidgets_LIBRARIES\ libexpat) doesn't match anything in file 'src/CMakeLists.txt'
Near pkgs/applications/misc/prusa-slicer/super-slicer.nix:83:

   |
83 |       inherit description;
   | ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md
warning: stale-substitute
Stale substituteInPlace detected.
substituteStream(): WARNING: pattern libexpat doesn't match anything in file 'src/libslic3r/CMakeLists.txt'
Near pkgs/applications/misc/prusa-slicer/super-slicer.nix:83:

   |
83 |       inherit description;
   | ^

See: https://github.com/jtojnar/nixpkgs-hammering/blob/master/explanations/stale-substitute.md

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

See below for a few thoughts, questions, and suggestions while taking a first glance at the changes in this PR…

pkgs/by-name/ss/sse2neon/package.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
@gador
Copy link
Member Author

gador commented Apr 30, 2024

Update:
Blender relies on "metal" support, which the current apple sdk doesn't provide for x86_64-darwin.
Updating to 11_0 yields even different errors and metal support should officially start at 11_2. My guess is, the sdk is just too old for the current blender.

I suggest revisting blender x86_64-darwin, after #229210 has been merged.
aarch64-darwin works (but without metal render support)

@afh
Copy link
Member

afh commented Apr 30, 2024

@gador out of a whim / "twinge of madness" I started work to add darwin-sdk-14. Is that something you might be interested in pursuing further together and to possibly propose it to #229210?

@afh
Copy link
Member

afh commented Apr 30, 2024

Slightly ambiguous miscommunication from my end, @gador: I cannot take any credit for any work done in #229210. I changes related to add support darwin-sdk-14 locally, but not published anywhere…

@gador
Copy link
Member Author

gador commented Apr 30, 2024

@afh ah ok. I don't think apple sdk 14 will bring much improvement for this PR, but if you have something, I'll test it against blender 👍

@gador
Copy link
Member Author

gador commented May 1, 2024

Result of nixpkgs-review pr 307833 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • blender-hip
10 packages built:
  • blender
  • materialx
  • openimagedenoise
  • openpgl
  • openusd
  • openvdb
  • openvdb.dev
  • prusa-slicer
  • python311Packages.openusd
  • sse2neon

Copy link
Member

@afh afh left a comment

Choose a reason for hiding this comment

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

A few thoughts, questions, and suggestions on this PR for you, @gador. Keep up the great work 👏, I'd love to see this PR get completed and merged.

pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/by-name/ma/materialx/package.nix Show resolved Hide resolved
pkgs/by-name/ma/materialx/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ss/sse2neon/package.nix Show resolved Hide resolved
pkgs/by-name/ss/sse2neon/package.nix Show resolved Hide resolved
pkgs/development/libraries/openimagedenoise/default.nix Outdated Show resolved Hide resolved
@gador gador force-pushed the blender-darwin branch 3 times, most recently from e445fd4 to 65c76dd Compare May 2, 2024 09:11
@gador
Copy link
Member Author

gador commented May 2, 2024

As far as I can see, all comments have been addressed and blender builds and works on aarch64-darwin now. I'll rebase and squash and will "undraft" this PR. Thanks for all the valuable input!

@github-actions github-actions bot added the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label May 2, 2024
gador and others added 4 commits May 2, 2024 11:12
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Co-authored-by: Alexis Hildebrandt <afh@surryhill.net>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
@github-actions github-actions bot removed the 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` label May 2, 2024
@gador gador marked this pull request as ready for review May 2, 2024 09:14
Copy link
Contributor

@ShaddyDC ShaddyDC left a comment

Choose a reason for hiding this comment

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

Sorry to drop by again so late with these minor points. I might have missed a spot where something wasn't inserted in order. The openusd changes still look good to me and worked fine. The rest looks fine to me, but I haven't tested it and am not familiar with the details.
Thank you for your work!

pkgs/by-name/ma/materialx/package.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/openusd/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
pkgs/applications/misc/blender/default.nix Outdated Show resolved Hide resolved
@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels May 7, 2024
gador and others added 3 commits May 7, 2024 21:22
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Co-authored-by: Alexis Hildebrandt <afh@surryhill.net>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
gador and others added 2 commits May 7, 2024 21:32
currently builds on aarch64-darwin, but not on x86_64-darwin.

Blender (specifically the ghost window library) depends on "metal" on
apple devices.
(For details see source/intern/ghost/intern/GHOST_System.cc line 416ff)

It cannot build without it. Metal support should be
introduced by apple sdk 11_1 (?) onward. Once this has been updated in
nixpkgs, this derivation can be revised.

Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
Co-authored-by: Alexis Hildebrandt <afh@surryhill.net>
Signed-off-by: Florian Brandes <florian.brandes@posteo.de>
@veprbl veprbl merged commit 7bfa087 into NixOS:master May 13, 2024
24 checks passed
@afh
Copy link
Member

afh commented May 14, 2024

Thanks for putting in all the hard work on this PR, @gador, very well done and very much appreciated 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: darwin Running or building packages on Darwin 6.topic: python 8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 12.approvals: 1 This PR was reviewed and approved by one reputable person 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants