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

itk: Adds RTK remote crate #353018

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

purepani
Copy link
Contributor

@purepani purepani commented Nov 1, 2024

Things done

ITK has a bunch of remote modules that can be used. This one enables RTK, which includes ct reconstruction. I haven't attempted adding more of them yet because this is the only one I need at the moment, but if I have time, they could be added. It might be better to get this merged first though.

As far as I can tell, building this module separately from itk is possible, but installing it is not supported: 3
https://github.com/RTKConsortium/RTK/blob/master/INSTALLATION.md

I have built it but haven't tested it yet(I am about to do so though).

Pinging @bcdarwin as a maintainer of this package.

  • 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.11 Release Notes (or backporting 23.11 and 24.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
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@purepani
Copy link
Contributor Author

purepani commented Nov 1, 2024

My bad i forgot to rebase
Edit: It's fixed now

Copy link
Member

@bcdarwin bcdarwin left a comment

Choose a reason for hiding this comment

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

Definitely a useful module to enable.

Some formatting nits:

  • please consider renaming to rtkSrc to match the other fixed-output derivations in this expression
  • please move to above swigUnstable next to the other fixed-output derivations and add an extra linebreak (unfortunately nixfmt doesn't deal with this)

@purepani
Copy link
Contributor Author

purepani commented Nov 2, 2024

Should be good now @bcdarwin!

Copy link
Member

@bcdarwin bcdarwin left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@ofborg ofborg bot requested a review from bcdarwin November 2, 2024 11:30
@wegank wegank added 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Nov 2, 2024
@purepani
Copy link
Contributor Author

purepani commented Nov 4, 2024

Hey @bcdarwin what else needs to be done to merge this? (I'm fine with waiting, but it's been a while since I contributed so I don't know if I need to get someone else or if it's in a pipeline somewhere to get merged)

@bcdarwin
Copy link
Member

bcdarwin commented Nov 4, 2024

Hey @bcdarwin what else needs to be done to merge this? (I'm fine with waiting, but it's been a while since I contributed so I don't know if I need to get someone else or if it's in a pipeline somewhere to get merged)

Nothing, someone with the commit bit just needs to see and merge it.

(But also see https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#practical-contributing-advice.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 11-100 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.

3 participants