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

rocwmma: init at 0.8-5.3.3 #202685

Merged
merged 1 commit into from
Nov 27, 2022
Merged

rocwmma: init at 0.8-5.3.3 #202685

merged 1 commit into from
Nov 27, 2022

Conversation

Madouura
Copy link
Contributor

@Madouura Madouura commented Nov 24, 2022

Description of changes

Tracking: #197885

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.

@Madouura Madouura mentioned this pull request Nov 24, 2022
34 tasks
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Nov 24, 2022
@ofborg ofborg bot requested a review from Flakebi November 24, 2022 14:50
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Nov 24, 2022
@Madouura Madouura force-pushed the pr/rocwmma branch 2 times, most recently from a27b610 to 1837627 Compare November 24, 2022 17:13
@ofborg ofborg bot requested a review from Flakebi November 24, 2022 17:17
@Madouura
Copy link
Contributor Author

Moved rocwmma in all-packages.nix to avoid conflict with rocrand

@ofborg ofborg bot requested a review from Flakebi November 25, 2022 17:33
@mweinelt mweinelt merged commit 5da55af into NixOS:master Nov 27, 2022
@Madouura Madouura deleted the pr/rocwmma branch November 27, 2022 13:05
Comment on lines +9 to +18
, gtest ? null
, rocblas ? null
, texlive ? null
, doxygen ? null
, sphinx ? null
, python3Packages ? null
, buildTests ? false
, buildSamples ? false
, buildDocs ? false
, gpuTargets ? null # gpuTargets = [ "gfx908:xnack-" "gfx90a:xnack-" "gfx90a:xnack+" ... ]
Copy link
Member

@SuperSandro2000 SuperSandro2000 Nov 27, 2022

Choose a reason for hiding this comment

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

There should not be added new instances where packages are being overwritten to null to disable features. This is an anti pattern and is totally not required here including the asserts below. This is a legacy artifact from before callPackage when import was still used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression it was to not include those packages and make the closure smaller.
If it's not, I'll change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

gpuTargets is however null for a different reason.
Is there a reason to change it?
Would it be better to change it to something like [ ]?

Copy link
Member

Choose a reason for hiding this comment

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

I was under the impression it was to not include those packages and make the closure smaller.

It is enough to use lib.optional(s) or if else then for that.

Would it be better to change it to something like [ ]?

That depends on how it should be handled. If only the entries in the list matter than having it by default an empty list is better defined. If there is a difference between null and empty list then it maybe shouldn't be changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is no discernible difference, so it should be fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants