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 the 'HiGHS' solver for linear programs #295386

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

silky
Copy link
Member

@silky silky commented Mar 12, 2024

Description of changes

Adds the HiGHS solver! :)

Things done

  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Mar 12, 2024
@silky silky force-pushed the noon/add-highs branch 2 times, most recently from 3c407ba to b644710 Compare March 12, 2024 18:56
Copy link
Contributor

@ConnorBaker ConnorBaker left a comment

Choose a reason for hiding this comment

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

Nicely done!

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

1 package built:
  • highs

A few suggestions, things to introduce you to, or make you aware of in case you weren't:

  1. Consider moving away from use of the rec keyword -- stdenv.mkDerivation accepts either an attribute set, a function to an attribute set (i.e. finalAttrs: {}) or a binary function to an attribute set (i.e. finalAttrs: prevAttrs: {}). This functionality was added specifically for this reason: stdenv.mkDerivation: overlay style overridable recursive attributes #119942. More on that discussion here: https://discourse.nixos.org/t/avoid-rec-expresions-in-nixpkgs/8293. In your case, I'd recommend using (finalAttrs: { ... }) instead of rec, and replacing rev = "v${version}"; with rev = "v${finalAttrs.version}";.
  2. Consider setting strictDeps = true; to enforce better separation of build-time and run-time dependencies (not enforced by default). It can help reduce runtime closures and is generally a blessing when doing cross-compilation of packages. It's not super important here, but a cool thing to know about!
  3. Consider quoting $out in installCheckPhase to avoid splitting on whitespace if someone has a funky NixOS store path.
  4. Consider adding yourself as a maintainer, if you're willing :)

IMHO, none of these are blocking and they're all minor suggestions or things I wanted to introduce you to or make you aware of since this is your first contribution -- let me know if you're ready for merge and I'd be happy to make that happen!

@ConnorBaker ConnorBaker self-assigned this Mar 12, 2024
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 12, 2024
@silky
Copy link
Member Author

silky commented Mar 12, 2024

Thanks @ConnorBaker ! I've made all the changes you suggested :) Much appreciated!

Looking forward to getting this merged :)

@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 12, 2024
@ofborg ofborg bot added the 11.by: package-maintainer This PR was created by the maintainer of the package it changes label Mar 12, 2024
@ConnorBaker ConnorBaker merged commit 2dd8b30 into NixOS:master Mar 13, 2024
21 checks passed
@silky silky deleted the noon/add-highs branch March 13, 2024 06:00
@KiaraGrouwstra KiaraGrouwstra mentioned this pull request Mar 16, 2024
13 tasks
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: 1-10 10.rebuild-darwin: 1 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 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants