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

coqPackages: refactor #105877

Merged
merged 1 commit into from Jan 9, 2021
Merged

coqPackages: refactor #105877

merged 1 commit into from Jan 9, 2021

Conversation

CohenCyril
Copy link
Contributor

@CohenCyril CohenCyril commented Dec 4, 2020

Motivation for this change

Use a mkCoqDerivation function to refactor Coq packages derivations.
This smart builder also allows for easier overriding of the source of the derivation.
See the documentation for more details.

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.

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages coqPackages_8_10 coqPackages_8_12

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages coqPackages_8_10 coqPackages_8_12

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages coqPackages_8_10 coqPackages_8_12

@CohenCyril
Copy link
Contributor Author

CohenCyril commented Dec 12, 2020

@vbgl @Zimmi48 this is ready modulo the doc.

@Zimmi48

This comment has been minimized.

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages.Cheerios coqPackages.CoLoR coqPackages.InfSeqExt coqPackages.QuickChick coqPackages.StructTact coqPackages.Verdi coqPackages.bignums coqPackages.contribs coqPackages.coq coqPackages.coq-bits coqPackages.coq-elpi coqPackages.coq-ext-lib coqPackages.coqeal coqPackages.coqhammer coqPackages.coqprime coqPackages.coquelicot coqPackages.dpdgraph coqPackages.equations coqPackages.flocq coqPackages.gappalib coqPackages.hierarchy-builder coqPackages.interval coqPackages.iris coqPackages.math-classes coqPackages.mathcomp coqPackages.mathcomp-algebra coqPackages.mathcomp-analysis coqPackages.mathcomp-bigenough coqPackages.mathcomp-character coqPackages.mathcomp-field coqPackages.mathcomp-fingroup coqPackages.mathcomp-finmap coqPackages.mathcomp-real-closed coqPackages.mathcomp-solvable coqPackages.mathcomp-ssreflect coqPackages.metalib coqPackages.multinomials coqPackages.paco coqPackages.paramcoq coqPackages.simple-io coqPackages.ssreflect coqPackages.stdpp coqPackages.tlc

@CohenCyril CohenCyril changed the title coqPackages refactoring coqPackages: refactor Jan 7, 2021
@CohenCyril
Copy link
Contributor Author

CohenCyril commented Jan 7, 2021

There is a left-over comment around the github fetcher.
The changes in this PR should be squashed into a single commit.
And then we are good to go!

@vbgl Done, can you double check if I did what you meant?

@ofborg ofborg bot requested a review from vbgl January 7, 2021 16:14
"-lablgtkdir ${ocamlPackages.lablgtk}/lib/ocaml/*/site-lib/lablgtk2 -coqide opt";
csdpPatch = if csdp != null then ''
substituteInPlace plugins/micromega/sos.ml --replace "; csdp" "; ${csdp}/bin/csdp"
substituteInPlace plugins/micromega/coq_micromega.ml --replace "System.is_in_system_path \"csdp\"" "true"
'' else "";
ocamlPackages = if isNull (args.ocamlPackages or null) then args.ocamlPackages else
Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t understand this line.

The mkCoq function below always set the ocamlPackages argument, so it is not null, therefore it is ignored and the default ocamlPackages set is computed depending on coq-version.

Suggestion: move the logic for computing the right version of OCaml out of the coq derivation (it thus becomes straightforward to overwrite).

Copy link
Contributor Author

@CohenCyril CohenCyril Jan 7, 2021

Choose a reason for hiding this comment

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

The mkCoq function makes a call to callPackage ../applications/science/logic/coq which leaves out coqPackages indeed, but callPackage provides the ability to override the arguments and hence one can write coq.override {ocamlPackages = whatever;} to change the version of ocaml used by Coq.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, I did two mistakes that cancel each other, sorry! And well spot!
I'll push a fix asap.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed my mistake and documented the new instructions... which are using an attribute customOcamlPackages.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like you are inventing yet an other overriding mechanism. I’m glad you did not duplicate all parameters (make & customMake, etc.). But I won’t argue against it.

Thanks for the fix nonetheless.

Copy link
Contributor Author

@CohenCyril CohenCyril Jan 8, 2021

Choose a reason for hiding this comment

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

There are two reasons combined that made me duplicate this predicate that would not possibly make me duplicate any other. 1. it's a scoped argument, which is passed implicitly by callPackage, so I cannot prevent it from being always set whereas I wanted it to be optional. 2. it's the only argument that is both a derivation and needs to change depending on the Coq version.

I don't see how this is yet another overriding mechanism, I'm merly reusing .override provided by callPackage.

Do you see another solution that would make it possible to have the following two derivation build correctly:

  • coq.override {version = "8.10"};
  • coq.override {version = "d370a9d1328a4e1cdb9d02ee032f605a9d94ec7a"; customOcamlPackages = ocaml-ng.ocamlPackages_4_09;};

Now I tried those two commands, I realize that the second one does not work anymore since I started using fetchzip everywhere. See my next comment.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I meant about this overriding scheme being ad-hoc, is that there is an argument whose only purpose is overriding. Notice that now there are two ways to override the ocamlPackages set: either by setting customOCamlPackages or by overriding, say, ocamlPackages_4_10.

@CohenCyril CohenCyril force-pushed the mathcomp_refactoring branch 2 times, most recently from 8c83d77 to 14ee56c Compare January 7, 2021 18:18
@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages.Cheerios coqPackages.CoLoR coqPackages.InfSeqExt coqPackages.QuickChick coqPackages.StructTact coqPackages.Verdi coqPackages.bignums coqPackages.contribs coqPackages.coq coqPackages.coq-bits coqPackages.coq-elpi coqPackages.coq-ext-lib coqPackages.coqeal coqPackages.coqhammer coqPackages.coqprime coqPackages.coquelicot coqPackages.dpdgraph coqPackages.equations coqPackages.flocq coqPackages.gappalib coqPackages.hierarchy-builder coqPackages.interval coqPackages.iris coqPackages.math-classes coqPackages.mathcomp coqPackages.mathcomp-algebra coqPackages.mathcomp-analysis coqPackages.mathcomp-bigenough coqPackages.mathcomp-character coqPackages.mathcomp-field coqPackages.mathcomp-fingroup coqPackages.mathcomp-finmap coqPackages.mathcomp-real-closed coqPackages.mathcomp-solvable coqPackages.mathcomp-ssreflect coqPackages.metalib coqPackages.multinomials coqPackages.paco coqPackages.paramcoq coqPackages.simple-io coqPackages.ssreflect coqPackages.stdpp coqPackages.tlc

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages_8_10 coqPackages_8_12 coqPackages_8_13

@ofborg ofborg bot requested a review from vbgl January 7, 2021 18:44
@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages.Cheerios coqPackages.CoLoR coqPackages.InfSeqExt coqPackages.QuickChick coqPackages.StructTact coqPackages.Verdi coqPackages.bignums coqPackages.contribs coqPackages.coq coqPackages.coq-bits coqPackages.coq-elpi coqPackages.coq-ext-lib coqPackages.coqeal coqPackages.coqhammer coqPackages.coqprime coqPackages.coquelicot coqPackages.dpdgraph coqPackages.equations coqPackages.flocq coqPackages.gappalib coqPackages.hierarchy-builder coqPackages.interval coqPackages.iris coqPackages.math-classes coqPackages.mathcomp coqPackages.mathcomp-algebra coqPackages.mathcomp-analysis coqPackages.mathcomp-bigenough coqPackages.mathcomp-character coqPackages.mathcomp-field coqPackages.mathcomp-fingroup coqPackages.mathcomp-finmap coqPackages.mathcomp-real-closed coqPackages.mathcomp-solvable coqPackages.mathcomp-ssreflect coqPackages.metalib coqPackages.multinomials coqPackages.paco coqPackages.paramcoq coqPackages.simple-io coqPackages.ssreflect coqPackages.stdpp coqPackages.tlc

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages_8_10 coqPackages_8_12 coqPackages_8_13

@ofborg ofborg bot requested a review from Zimmi48 January 8, 2021 02:06
@CohenCyril
Copy link
Contributor Author

CohenCyril commented Jan 8, 2021

Since I started using fetchzip everywhere, I lost the ability to fetch arbitrary sources without providing a hash, for unreleased versions. There are two ways I could fix this:

  1. for unreleased versions, revert to fetchTarball in the default fetcher, which supports downloads without hashes,
  2. make it necessary to provide a hash even for unreleased version, using e.g.
   mathcomp.override {version = "#682"; sha256 = "<the sha256>";}

but this comes with extra burden for the casual users since there will be no more easy setups mentioning only a branch...

@vbgl and @Zimmi48 what do you think? Do you see other solutions?

EDIT: I opted for 1. in a quite compact way, we'll see what comes out of this...

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages.Cheerios coqPackages.CoLoR coqPackages.InfSeqExt coqPackages.QuickChick coqPackages.StructTact coqPackages.Verdi coqPackages.bignums coqPackages.contribs coqPackages.coq coqPackages.coq-bits coqPackages.coq-elpi coqPackages.coq-ext-lib coqPackages.coqeal coqPackages.coqhammer coqPackages.coqprime coqPackages.coquelicot coqPackages.dpdgraph coqPackages.equations coqPackages.flocq coqPackages.gappalib coqPackages.hierarchy-builder coqPackages.interval coqPackages.iris coqPackages.math-classes coqPackages.mathcomp coqPackages.mathcomp-algebra coqPackages.mathcomp-analysis coqPackages.mathcomp-bigenough coqPackages.mathcomp-character coqPackages.mathcomp-field coqPackages.mathcomp-fingroup coqPackages.mathcomp-finmap coqPackages.mathcomp-real-closed coqPackages.mathcomp-solvable coqPackages.mathcomp-ssreflect coqPackages.metalib coqPackages.multinomials coqPackages.paco coqPackages.paramcoq coqPackages.simple-io coqPackages.ssreflect coqPackages.stdpp coqPackages.tlc

@CohenCyril
Copy link
Contributor Author

@GrahamcOfBorg build coqPackages_8_10 coqPackages_8_12 coqPackages_8_13

@ofborg ofborg bot requested a review from vbgl January 8, 2021 18:11
@Zimmi48
Copy link
Member

Zimmi48 commented Jan 8, 2021

I'm definitely in favor of option 1, i.e. not making the hash compulsory. However, I missed the reason why you didn't use fetchTarball for everything.

@CohenCyril
Copy link
Contributor Author

CohenCyril commented Jan 8, 2021

I'm definitely in favor of option 1, i.e. not making the hash compulsory. However, I missed the reason why you didn't use fetchTarball for everything.

@Zimmi48 cf #105877 (comment) and subsequent discussion
fetchTarball prevents evaluation by GrahamcOfBorg for some reason.

@CohenCyril
Copy link
Contributor Author

@vbgl I guess everything should be solved now...

@vbgl vbgl merged commit 9ffd16b into NixOS:master Jan 9, 2021
@siraben
Copy link
Member

siraben commented Jan 10, 2021

It appears that it is not rendering correctly in the generated documentation (huge wall of text rather than being a list), https://nixos.org/manual/nixpkgs/unstable/#sec-language-coq

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

4 participants