Conversation
github-actions
bot
added
10.rebuild-darwin: 0
pkgs/by-name/bl/blender/package.nix
Outdated
There was a problem hiding this comment.
--replace is deprecated, so change these both to --replace-fail, which will also make the problem a build time instead of runtime failure. While we’re at it, make it dynamic so we are less likely to need to manually change it later.
| substituteInPlace extern/hipew/src/hipew.c --replace '"/opt/rocm/hip/lib/libamdhip64.so.6"' '"${rocmPackages.clr}/lib/libamdhip64.so"' | |
| substituteInPlace extern/hipew/src/hipew.c --replace '"opt/rocm/hip/bin"' '"${rocmPackages.clr}/bin"' | |
| substituteInPlace extern/hipew/src/hipew.c --replace-fail '"/opt/rocm/hip/lib/libamdhip64.so.${lib.versions.major rocmPackages.clr.version}"' '"${rocmPackages.clr}/lib/libamdhip64.so"' | |
| substituteInPlace extern/hipew/src/hipew.c --replace-fail '"opt/rocm/hip/bin"' '"${rocmPackages.clr}/bin"' |
There was a problem hiding this comment.
I did make those replace-fail changes locally, but wanted to keep the PR as minimal as possible (doh!). Interestingly enough, replace-fail wouldn't have caught this issue, since upstream changed the /opt/rocm/hip/lib/libamdhip64.so to /opt/rocm/hip/lib/libamdhip64.so.6, so it would still have replaced /opt/rocm/hip/lib/libamdhip64.so with our expression and then it would have had .6 after it and we don't produce libamdhip64.so.6 with our derivation. Not sure of the usefulness of making that soversion dynamic, there is no predicting what blender devs will do with this detection code in the future. I mean, this PR is necessary in the first place because they made a weird change to it (4.4 changelog says they dropped rocm <6 support, but they added backwards compat code anyway?)
There was a problem hiding this comment.
replace-fail wouldn't have caught this issue…
It would have because the string to substitute matches the ending ", so .so.6" would not have matched. Changing just --replace → --replace-fail on nixos-unstable causes the build to fail here.
but wanted to keep the PR as minimal as possible
Some minor related cleanup is fine. I think in this case it’s not unreasonable to have in the same commit, but a separate commit in the same PR is also fine.
Not sure of the usefulness of making that soversion dynamic, there is no predicting what blender devs will do
Sure, but I suspect the most likely thing will be .so.6 → .so.7 given the current code. Further, it helps somewhat ensure that we are using a matching ROCm, as if our ROCm changes to v7 but Blender still only references v6, we might have an issue that needs addressing.
There was a problem hiding this comment.
Good catch, I might need to change my font settings, I didn't catch that "' thing at all. I'll revise the PR later and force push with your suggestions incorporated. I also have a new hiprt package and accompanying blender enablement patch that I might add in a separate PR
There was a problem hiding this comment.
Good catch, I might need to change my font settings, I didn't catch that "' thing at all. I'll revise the PR later and force push with your suggestions incorporated. I also have a new hiprt package and accompanying blender enablement patch that I might add in a separate PR
I'm trying to package hip-rt here #411736. Although I'm facing some issues. If you've already fixed them we should replace it with yours.
|
Hi |
|
I'll make the suggested changes and look at your hiprt tomorrow most likely |
|
should be fine now, not sure about the nixpkgs-vet failing check, should I have rebased onto master again? |
Yeah, looks like a spurious failure unrelated to your diff that was fixed in #413239 |
5 participants
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.