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

sage: fix doctest matrix_double_dense is_unitary #86274

Merged
merged 1 commit into from May 1, 2020
Merged

sage: fix doctest matrix_double_dense is_unitary #86274

merged 1 commit into from May 1, 2020

Conversation

samlich
Copy link
Contributor

@samlich samlich commented Apr 29, 2020

Motivation for this change

sage-tests fails due to matrix_double_dense is_unitary test https://hydra.nixos.org/build/117853043/nixlog/1

Things done

Added patch from upstream to fix test

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-review wip"
  • 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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@samlich samlich requested a review from timokau April 29, 2020 07:19
@timokau
Copy link
Member

timokau commented Apr 29, 2020

Neat, I didn't find the time to fix that yet. Thank you.

Where is that upstream patch coming from? Please add a link to the trac ticket. I'm somewhat surprised there's an upstream fix to this, as I assumed the breakage was due to the recent blas infrastructure changes (#83888).

@samlich
Copy link
Contributor Author

samlich commented Apr 30, 2020

https://trac.sagemath.org/ticket/29297
They comment about the failure being triggered by changing a library, so that probably was the cause.

@timokau
Copy link
Member

timokau commented Apr 30, 2020

Ah, so it is related to the blas version. We should probably figure out why #83888 changed the blas sage uses. But this works for now.

Please add the link to the trac ticket as a source comment, then this should be good to go.

@timokau
Copy link
Member

timokau commented Apr 30, 2020

Maybe also mention the blas change responsible for the error or just link this PR. As a general rule of thumb, whoever reads the .nix file should have enough context to determine why a patch was added and when it can be removed.

@samlich
Copy link
Contributor Author

samlich commented May 1, 2020

D'oh, now it's actually in the code.

@timokau
Copy link
Member

timokau commented May 1, 2020

Great, thanks for taking care of this!

@timokau timokau merged commit 7866440 into NixOS:master May 1, 2020
@samlich samlich deleted the sage-matrix-double-dense branch May 3, 2020 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timokau timokau Awaiting requested review from timokau

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@samlich @timokau