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

coq_8_14: init at 8.14+rc1 #138274

Merged
merged 2 commits into from
Oct 13, 2021
Merged

coq_8_14: init at 8.14+rc1 #138274

merged 2 commits into from
Oct 13, 2021

Conversation

Zimmi48
Copy link
Member

@Zimmi48 Zimmi48 commented Sep 17, 2021

Motivation for this change

Coq 8.14+rc1 was just tagged.

Things done

Currently testing this, but I expect this will work given that the Coq derivation has been 8.14-ready for a while now.

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 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.

cc @vbgl

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Sep 17, 2021
@ofborg ofborg bot requested review from roconnor, vbgl and thoughtpolice September 17, 2021 13:14
@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

coq_8_14 does build fine on my machine. I'm currently testing at https://github.com/coq-community/coq-nix-toolbox/actions/runs/1245604615 if there are packages in the already quite large coqPackages_8_14 set that are not actually buildable.

@vbgl
Copy link
Contributor

vbgl commented Sep 17, 2021

Build failure (is which missing from the nativeBuildInputs?):

dev/tools/make_git_revision.sh: line 3: which: command not found

Also coqPackages_8_14.addition-chains fails to evaluate; suggested patch:

--- a/pkgs/development/coq-modules/addition-chains/default.nix
+++ b/pkgs/development/coq-modules/addition-chains/default.nix
@@ -11,7 +11,7 @@ mkCoqDerivation {
 
   inherit version;
   defaultVersion = with versions; switch coq.coq-version [
-    { case = isGe "8.11"; out = "0.4"; }
+    { case = range "8.11" "8.13"; out = "0.4"; }
   ] null;
 
   propagatedBuildInputs = [ mathcomp-ssreflect mathcomp-algebra paramcoq ];

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

Is the problem really that which is missing (I think this is just a warning) or this line: Error: no rule to make target bin/votour (or missing .PHONY)? How come it didn't fail when I built it on my side?

@vbgl
Copy link
Contributor

vbgl commented Sep 17, 2021

Indeed, the bin/votour missing target seems to be the actual error. How does it only happen on CI?

BTW, Cheerios, Verdi, CoqEAL still fail to evaluate.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

dev/tools/make_git_revision.sh: line 3: which: command not found can indeed also been seen in the log of the successful build at: https://github.com/coq-community/coq-nix-toolbox/pull/65/checks?check_run_id=3632607063. For some reason, in this case this is a different derivation that is built. However, on my system, I have checked that the derivation that I've built is the same as the one that has failed in CI here. So, that's strange indeed.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

I also got this line in the log linked above: make[1]: 'bin/votour' is up to date.

@Zimmi48 Zimmi48 force-pushed the coq-8-14 branch 2 times, most recently from 4753333 to dbce3f9 Compare September 17, 2021 14:48
@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

It's a bit annoying to have to manually tweak the defaultVersion computation of packages just because they have dependencies that are marked as broken (even if they might actually work fine with Coq 8.14 if we update the dependency). Would it be possible to automatically mark as broken the reverse dependencies of a broken package?

@vbgl
Copy link
Contributor

vbgl commented Sep 17, 2021

You may leave out the evaluation failures for now and just be sure that they get fixed before 8.14.0 gets in nixpkgs.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 17, 2021

Yes, but I also want CI on the coq-nix-toolbox repo to pass fully, so I'll fix all the evaluation failures.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 19, 2021

IIUC the only blocking thing to merge this PR would be to understand the CI failure, but despite the failure being reproducible on ofborg runs, it is truly incomprehensible since it never occurred while building locally or on the coq-nix-toolbox CI (and I have checked that the derivations being built are the same). Are there some known outside parameters that can influence how a derivation is built? I believe that my initial build was sandboxed as it ran on my NixOS machine.

@SkySkimmer
Copy link

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 22, 2021

Thanks for your suggestion. I will make it conditional. But I still fail to understand why the same version was building fine elsewhere.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 22, 2021

Rebuilding coq_8_14 after this change did not trigger a new build on my machine, which seems to indicate that the derivation which was build was already ignoring this target (or the buildFlags entirely).

Nah, that was my conditional which was backwards.

@ofborg ofborg bot requested a review from jwiegley September 22, 2021 19:50
@vbgl
Copy link
Contributor

vbgl commented Sep 28, 2021

Oh, nice. I did not notice that you had fixed it.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 28, 2021

That's new from yesterday 😉

FTR, here is the current structure of result:

  • bin/
  • lib/coq/theories
  • lib/coq/user-contribs
  • lib/coq-core/META (and OCaml libs)
  • lib/coqide/META
  • lib/coqide-server/META (and OCaml libs)
  • lib/ocaml/4.10.2/site-lib/coq-core (symbolic link to lib/coq-core)
  • lib/stublibs
  • nix-support
  • share/applications
  • share/coq
  • share/doc (with just LICENSE and README files under coq-core, etc.)
  • share/man/man1
  • share/pixmaps
  • share/texmf/tex/latex/misc

Do you see anything to fix? From my side, all I noticed is that there should probably also be a symlink from lib/ocaml/4.10.2/site-lib/coqide-server to lib/coqide-server.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 28, 2021

From my side, all I noticed is that there should probably also be a symlink from lib/ocaml/4.10.2/site-lib/coqide-server to lib/coqide-server.

Now fixed.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 28, 2021

Something has been broken with the darwin build for a while, although previously in a different way.
eg https://github.com/NixOS/nixpkgs/runs/1959980556 (coq_8_13: 8.13.0 → 8.13.1) has no log just "This check concluded as stale."

It seems that this happens because ofborg will sometimes never run these Darwin tests. And this seems to be happening again now.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 28, 2021

FTR I did some tests on macOS via the Coq Nix Toolbox CI, and there doesn't seem to be any issue building Coq. Could there be again some discrepancy between how CI behaves when run by ofborg or in the GitHub Actions from the Coq Nix Toolbox? Hard to tell if the CI here doesn't complete.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 30, 2021

The ofborg CI tests on darwin have finally run, and they have completed with a failure. Meanwhile, the same tests ran in the GitHub Actions CI of the Coq Nix Toolbox and have completed successfully.

Once again, building the same derivation was attempted in both cases:

/nix/store/sq9wmdfqyppqbhzfrhqv1sc7i03a8f1q-coq-8.14+rc1.drv

The failure in ofborg tests is deterministic, it is always:

trying https://github.com/coq/coq/archive/V8.14+rc1.zip
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   117  100   117    0     0    609      0 --:--:-- --:--:-- --:--:--   609
100 8749k    0 8749k    0     0  5208k      0 --:--:--  0:00:01 --:--:-- 9053k
unpacking source archive /private/tmp/nix-build-source.drv-0/V8.14+rc1.zip
checkdir error:  cannot create coq-8.14-rc1/test-suite/misc/deps/+?+?
                 Illegal byte sequence
                 unable to process coq-8.14-rc1/test-suite/misc/deps/+?+?/.
checkdir error:  cannot create coq-8.14-rc1/test-suite/misc/deps/+?+?
                 Illegal byte sequence
                 unable to process coq-8.14-rc1/test-suite/misc/deps/+?+?/+?+?.v.
checkdir error:  cannot create coq-8.14-rc1/test-suite/misc/deps/+?+?
                 Illegal byte sequence
                 unable to process coq-8.14-rc1/test-suite/misc/deps/+?+?/+?+?.v.
do not know how to unpack source archive /private/tmp/nix-build-source.drv-0/V8.14+rc1.zip
builder for '/nix/store/p66rcxr6g0q9sn57spbgmn4qq7vsn95d-source.drv' failed with exit code 1

Similarly, the failure we were getting on Linux before fixing the build targets was also deterministic (and was not observed in the GitHub Actions CI of the Coq Nix Toolbox, despite the two buildings the same derivation).

I'm quite annoyed by this discrepancy, which is not supposed to happen with Nix. Could the explanation be that the two use a different version of Nix? The Coq Nix Toolbox CI uses Nix 2.3.15, but I don't know how to tell which version the ofborg CI uses.

@Zimmi48
Copy link
Member Author

Zimmi48 commented Sep 30, 2021

cc @grahamc in case you are able to help with this (see comment just above)

@Zimmi48
Copy link
Member Author

Zimmi48 commented Oct 1, 2021

So, it's actually not a Nix version problem. I have been able to reproduce the problem observed here in the GitHub Actions CI with Nix 2.3.15. See https://github.com/Zimmi48/coq-nix-toolbox/runs/3771003867?check_suite_focus=true (not explicitly pinned) and https://github.com/Zimmi48/coq-nix-toolbox/runs/3770951230?check_suite_focus=true (explicitly pinned).
Now, what I don't understand is what is the difference that made /nix/store/sq9wmdfqyppqbhzfrhqv1sc7i03a8f1q-coq-8.14+rc1.drv build successfully in https://github.com/coq-community/coq-nix-toolbox/runs/3730072982?check_suite_focus=true but consistently fail in other runs (like the two linked above).

@Zimmi48
Copy link
Member Author

Zimmi48 commented Oct 4, 2021

Now, what I don't understand is what is the difference that made /nix/store/sq9wmdfqyppqbhzfrhqv1sc7i03a8f1q-coq-8.14+rc1.drv build successfully in https://github.com/coq-community/coq-nix-toolbox/runs/3730072982?check_suite_focus=true but consistently fail in other runs (like the two linked above).

I'm thinking the explanation might simply be that the archive to download is the same on Linux and Darwin. So it suffices that the archive has already been downloaded on Linux and cached in NixOS official cache (or some Cachix) to make it available to derivations building Coq on Darwin. If that's the case, then it alleviates the problem for users (and explains that it wasn't noticed when running CI with the coq-community Cachix, which already contained the derivation output on Linux).

Does that make sense?

In any case, this issue does not seem to be a regression from 8.14 but rather a problem that was introduced with the switch from fetchTarball to fetchzip in #105877. We should discuss how to resolve it independently of this PR which has been delayed for too long already.

@vbgl Do you agree?

Copy link
Contributor

@vbgl vbgl left a comment

Choose a reason for hiding this comment

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

The multinomials library at version 1.5.4 is compatible with Coq 8.14.

pkgs/development/coq-modules/coqeal/default.nix Outdated Show resolved Hide resolved
@ofborg ofborg bot requested a review from vbgl October 11, 2021 17:18
{ case = range "8.8" "8.12"; out = "4.0.0"; }
{ case = range "8.7" "8.11"; out = "3.4.2"; }
{ case = range "8.8" "8.13"; out = "4.3.0"; }
{ case = isEq "8.7"; out = "3.4.2"; }
{ case = range "8.5" "8.6"; out = "3.3.0"; }
] null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please drop this change from this PR (unrelated and arguably wrong).

Copy link
Member Author

@Zimmi48 Zimmi48 Oct 12, 2021

Choose a reason for hiding this comment

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

The entire change to this file or the second line? If that's the second line, then I can certainly revert that but my point for doing this kind of change was that defaultVersion is not about providing "correct" version constraints but about having a way to decide which version to use based on the version of Coq and having overlapping ranges is not helpful in this respect (it rather makes the code less clear). (Similarly for the removed case that was just never reached.)

Copy link
Contributor

@vbgl vbgl Oct 12, 2021

Choose a reason for hiding this comment

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

I meant the entire set of changes made to this file.

As your reply suggests, it is worth discussion and unrelated to Coq 8.14.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, interval builds fine with Coq 8.14. I dropped the changes to this file.

@ofborg ofborg bot requested a review from vbgl October 12, 2021 09:44
@Zimmi48
Copy link
Member Author

Zimmi48 commented Oct 13, 2021

@vbgl anything else?

@vbgl vbgl merged commit e7735f2 into NixOS:master Oct 13, 2021
@Zimmi48 Zimmi48 deleted the coq-8-14 branch October 13, 2021 18:44
@Zimmi48
Copy link
Member Author

Zimmi48 commented Oct 14, 2021

With Cyril, we've discussed that we should report the issues with non-reproducibility of derivation building that we've encountered while preparing this PR (the bin/votour issue and the macOS-specific failure to download the source tarball) but I don't know when I'll find sufficient time to sit down and write a proper report for this. Probably not for a while.

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.

3 participants