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.

Sign up for GitHub

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

Jump to bottom

minc2_simple: init at unstable-2019-11-12 #72038

Closed
wants to merge 2 commits into from
Closed

minc2_simple: init at unstable-2019-11-12 #72038

wants to merge 2 commits into from

Conversation

bcdarwin
Copy link
Member

Motivation for this change

Add a package. Fresh version of #51094.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • [NA] Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • [NA] Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • [NA] Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • [NA] Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
Notify maintainers

cc @

@bcdarwin bcdarwin force-pushed the minc2_simple branch 2 times, most recently from 54dada4 to 0b04e29 Compare October 26, 2019 20:37
@bcdarwin
Copy link
Member Author

@jonringer -- all resolved, thanks.

@bcdarwin
Copy link
Member Author

@jonringer -- sure, done.

@bcdarwin bcdarwin changed the title minc2_simple: init at 2018-12-20 minc2_simple: init at unstable-2018-12-20 Oct 26, 2019
jonringer
jonringer reviewed Oct 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment •
edited

Choose a reason for hiding this comment

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

where are the headers?

[nix-shell:/home/jon/.cache/nix-review/pr-72038-1]$ tree ./results/minc2_simple/
./results/minc2_simple/
└── lib
    ├── libminc2-simple.so
    └── libminc2-simple-static.a

1 directory, 2 files

there should be a

  include/minc2-simple.h
  include/minc2-matrix-ops.h

@bcdarwin
Copy link
Member Author

Added.

@bcdarwin
Copy link
Member Author

Done

jonringer
jonringer approved these changes Oct 26, 2019
View reviewed changes
Copy link
Contributor

@jonringer jonringer left a comment

Choose a reason for hiding this comment

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

nix-review passes on NixOS
diff LGTM
commits LGTM
repo looks relatively active

[5 built, 0.0 MiB DL]
https://github.com/NixOS/nixpkgs/pull/72038
4 package were build:
minc2_simple python27Packages.minc2_simple python37Packages.minc2_simple python38Packages.minc2_simple

@jonringer
Copy link
Contributor

@GrahamcOfBorg build minc2_simple python27Packages.minc2_simple python37Packages.minc2_simple python38Packages.minc2_simple

@jonringer
Copy link
Contributor

it looks like there's failure on aarch, may want to put platforms to "x86_64_linux", don't want hydra to constantly try to rebuild an unsupported package

@bcdarwin
Copy link
Member Author

Is there a way to do this other than platforms = stdenv.lib.intersectLists platforms.linux platforms.x86_64;?

risicle
risicle reviewed Oct 27, 2019
View reviewed changes
pkgs/development/libraries/minc2-simple/default.nix
'';

# tests don't actually run automatically, e.g., missing input files
doCheck = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

A medical imaging library does seem like the sort of package we should at least make an effort to enable the tests for...

Copy link
Member Author

Choose a reason for hiding this comment

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

I will work with upstream on this, but note that the tests for the Python interface to this library pass.

@bcdarwin
Copy link
Member Author

@jonringer @risicle - upstream has merged my changes to automatically install the headers so I've updated to the most recent commit. Tests are still broken upstream but I've filed an issue (but note the Python tests which depend on the C library work correctly.)

@bcdarwin bcdarwin changed the title minc2_simple: init at unstable-2018-12-20 minc2_simple: init at unstable-2019-11-12 Jan 16, 2020
@bcdarwin bcdarwin closed this Jan 27, 2020
@bcdarwin bcdarwin deleted the minc2_simple branch March 22, 2023 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@risicle risicle risicle left review comments

@jonringer jonringer jonringer approved these changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@bcdarwin @jonringer @risicle