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

hip: fix cmake installation directories #197838

Merged
merged 1 commit into from
Nov 3, 2022
Merged

Conversation

Madouura
Copy link
Contributor

@Madouura Madouura commented Oct 26, 2022

Description of changes

Tracking: #197885
Turns out any ${hip}/hip paths were turning into ${hip}/${hip} paths in hip-config.cmake.
This fixes that, and removed the need for the workaround on my other PRs.

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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@LunNova
Copy link
Member

LunNova commented Oct 27, 2022

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

2 packages built:
  • blender-hip
  • hip

@LunNova
Copy link
Member

LunNova commented Oct 27, 2022

Builds but not sure how to test blender properly so can't really properly review it.

@Madouura
Copy link
Contributor Author

The only real change is making sure the symlinks in ${hip}/hip correctly point to their corresponding directories in the root folder.
In other words, it's probably fine. It does work I know for sure with my other PRs using hip.

@@ -154,6 +154,9 @@ stdenv.mkDerivation rec {
"-DHIP_COMMON_DIR=${hip}"
"-DROCCLR_PATH=${rocclr}"
"-DHIP_VERSION_BUILD_ID=0"
"-DCMAKE_INSTALL_BINDIR=bin"
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@Madouura Madouura Oct 27, 2022

Choose a reason for hiding this comment

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

Upstream hip or upstream cmake?
This doesn't seem to happen with all roc* projects (rocprim, hipcub), but I have observed 3 others create ${nix_path}/${package_name}/${nix_path}.
For hip, this seems to result in bad symlinks instead.
The problem seems to be nix-specific in that we use /nix/store to store package roots, and amd's cmake projects in particular end up taking the full path instead of a relative path.

Copy link
Member

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's an issue for now. ROCm/hipamd#55

@Madouura Madouura changed the title hip: fix cmake installation directories hip: fix cmake installation directories (5.3.0 -> 5.3.1) Oct 30, 2022
@Madouura Madouura changed the title hip: fix cmake installation directories (5.3.0 -> 5.3.1) hip: fix cmake installation directories Oct 31, 2022
Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

LGTM, hip works for me with the patch and the dangling symlinks arer gone.

Copy link
Member

@LunNova LunNova left a comment

Choose a reason for hiding this comment

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

Looks good, should add a comment mentioning the upstream issue so we remember to remove those once it's fixed.

pkgs/development/compilers/hip/default.nix Show resolved Hide resolved
@Madouura
Copy link
Contributor Author

Madouura commented Nov 1, 2022

Hold off on merging while I investigate just disabling BUILD_FILE_REORG_BACKWARD_COMPATIBILITY in this and other packages.

@Madouura
Copy link
Contributor Author

Madouura commented Nov 3, 2022

This is okay to merge again, as unfortunately BUILD_FILE_REORG_BACKWARD_COMPATIBILITY and some other things didn't quite work out.
I'll have the rest of the packages in #197885 reference this issue as to why we're defining manual install directories.

Copy link
Member

@Flakebi Flakebi left a comment

Choose a reason for hiding this comment

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

Still LGTM

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.

None yet

5 participants