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

Add sundials #36526

Merged
merged 3 commits into from Mar 13, 2018
Merged

Add sundials #36526

merged 3 commits into from Mar 13, 2018

Conversation

idontgetoutmuch
Copy link
Contributor

Motivation for this change

A good package for solving differential equations that ought to be in nix.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@jfrankenau
Copy link
Member

Please change the commit message to sundials: init at 3.1.0 and add yourself as a maintainer as per CONTRIBUTING.md. The license is missing as well.

};

preConfigure = ''
export cmakeFlags="-DCMAKE_INSTALL_PREFIX=$out -DEXAMPLES_INSTALL_PATH=$out/share/examples $cmakeFlags"
Copy link
Contributor

Choose a reason for hiding this comment

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

You can say cmakeFlags = ... as a top-level attribute rather than setting it in preConfigure I believe.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that should be possible. See here for an example.

@idontgetoutmuch
Copy link
Contributor Author

The example also has # since we cant expand $out in cmakeFlags and I think I need $out?

# since we cant expand $out in cmakeFlags

@jfrankenau
Copy link
Member

Hah, should've looked further. You could try this out. Maybe it works now.

Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

Please rebase this PR to a current version of master to resolve the merge conflicts.

@GrahamcOfBorg GrahamcOfBorg removed 6.topic: emacs 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang labels Mar 11, 2018
@idontgetoutmuch
Copy link
Contributor Author

It seems I am not a maintainer?

description = "Suite of nonlinear differential/algebraic equation solvers";
homepage = https://computation.llnl.gov/casc/sundials/main.html;
platforms = platforms.all;
maintainers = [ idontgetoutmuch ];
Copy link
Member

Choose a reason for hiding this comment

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

You need to add an appropriate attribute in maintainers/maintainer-list.nix and refer to that in the maintainers field.

Copy link
Member

Choose a reason for hiding this comment

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

Please note that you must refer to maintainers.idontgetoutmuch here.

homepage = https://computation.llnl.gov/casc/sundials/main.html;
platforms = platforms.all;
maintainers = [ idontgetoutmuch ];
licencse = { url = https://computation.llnl.gov/projects/sundials/license; };
Copy link
Member

Choose a reason for hiding this comment

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

Typo here: licencse -> license. Also that looks like a three-clause BSD license and thus can be referenced by licenses.bsd3.

@idontgetoutmuch
Copy link
Contributor Author

@peti what else do I need to do?

@peti peti merged commit 54ade27 into NixOS:master Mar 13, 2018
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