Skip to content

Comments

mpfi: fix download URL#120614

Merged
7c6f434c merged 1 commit intoNixOS:masterfrom
eduardosm:mpfi
Apr 26, 2021
Merged

mpfi: fix download URL#120614
7c6f434c merged 1 commit intoNixOS:masterfrom
eduardosm:mpfi

Conversation

@eduardosm
Copy link
Contributor

@eduardosm eduardosm commented Apr 25, 2021

Motivation for this change

The file_nr value was wrong, so it was downloading version 1.5.3 instead of 1.5.4.

Things done
  • 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.

The `file_nr` value was wrong, so it was downloading version 1.5.3 instead of 1.5.4.
@ofborg ofborg bot requested a review from 7c6f434c April 25, 2021 17:04
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 25, 2021
@eduardosm
Copy link
Contributor Author

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

2 packages failed to build:
  • sage (sagemath)
  • sageWithDoc
4 packages built:
  • giac
  • giac-with-xcas
  • mpfi
  • sollya

@7c6f434c
Copy link
Member

@collares @omasanori @timokau any idea whether fixing the version bump is expected to break Sage? (or is it just an unrelated dependency failure?)

@eduardosm
Copy link
Contributor Author

It looks like sage is currently failing to build on master too.

@eduardosm
Copy link
Contributor Author

sage build logs:

@collares
Copy link
Member

collares commented Apr 25, 2021

Indeed, Sage is broken on master (probably due to the FLINT upgrade that happened a week ago). #116365 will unbreak it. Sage 9.3 ships with mpfi 1.5.2, but I don't think upgrading to 1.5.4 will be a big problem. If it is we can deal with it on the Sage side. Thanks for the heads-up!

@collares
Copy link
Member

Wait, no, I am wrong: 4 tests fail on master, while a whole bunch of tests fail in the above log. If it's possible to wait a bit, landing #116365 first would save Sage a small headache.

@collares collares mentioned this pull request Apr 25, 2021
14 tasks
@eduardosm
Copy link
Contributor Author

I get lots of AttributeError: 'cypari2.gen.Gen' object has no attribute 'bnf_get_fu' when building locally, but I have no idea why. I'll run nixpkgs-review on #116365.

@collares
Copy link
Member

collares commented Apr 25, 2021

Ah, I see, Sage's cypari2 was pinned to 2.1.1 to aid with the staging-next landing, but we migrated to 2.1.2 on master in the meantime and those changes conflicted with each other. Reverting commit c12ef05 should fix those bnf_get_fu errors.

@eduardosm If you do have a beefy machine, then a good way to ensure Sage is working is to revert c12ef05 and see if you only get 4 files with failing tests in Hydra (or testing on top of #116365). But if I had to bet, I would bet on the mpfi upgrade being safe, so if you don't have the resources to test Sage feel free to proceed with this PR.

@eduardosm
Copy link
Contributor Author

I'll test on top of #116365 and report back.

@7c6f434c
Copy link
Member

I bought the argument that «Sage on master is so broken, a bump to RC is an improvement». @collares it looks like you are currently the one with the best understanding of what is going on on the Sage side…

@collares
Copy link
Member

collares commented Apr 25, 2021

@7c6f434c Thanks for merging! I opened #120648 for the cypari2 conflict (the problem started happening today with the staging-next merge), and after that Sage should be working on master again, so the tests for this PR should be reliable.

@eduardosm
Copy link
Contributor Author

After applying #116365 and #120648, this PR will build fine.

@collares
Copy link
Member

@eduardosm Many thanks for testing!

@7c6f434c 7c6f434c merged commit 8931b66 into NixOS:master Apr 26, 2021
@eduardosm eduardosm deleted the mpfi branch April 26, 2021 09:31
@timokau
Copy link
Member

timokau commented Apr 29, 2021

Thanks for fixing this @eduardosm. The nixpkgs-update bot makes the file_nr disclaimer easier to miss. We could opt-out for this package, but there is very little harm done either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants