Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

coqPackages.coqeal: master, add dependency #142858

Merged
merged 1 commit into from
Nov 2, 2021
Merged

coqPackages.coqeal: master, add dependency #142858

merged 1 commit into from
Nov 2, 2021

Conversation

proux01
Copy link
Contributor

@proux01 proux01 commented Oct 25, 2021

In order to include matrix normal forms in CoqEAL (coq-community/coqeal#54) we add a dependency on mathcomp-real-closed for master.

Motivation for this change
Things done
  • 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.

@proux01
Copy link
Contributor Author

proux01 commented Oct 25, 2021

@cohen of @vbgl I may need your help here

@Zimmi48
Copy link
Member

Zimmi48 commented Oct 26, 2021

I would tend to disagree with @CohenCyril on the unconditional dependency addition. Here is an example package which adds a dependency depending on the version that is being built: https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/coq-modules/extructures/default.nix

However, instead of using versionAtLeast ..., use lib.versions.isGt "1.0" o.version || o.version == "dev" (assuming that the next version of CoqEAL will be at least 1.1 given the feature addition).

@CohenCyril
Copy link
Contributor

However, instead of using versionAtLeast ..., use lib.versions.isGt "1.0" o.version || o.version == "dev" (assuming that the next version of CoqEAL will be at least 1.1 given the feature addition).

Yes, I just had the problem with it, and I did not have the patience to explain how to solve it. Sorry.
cf https://github.com/math-comp/math-comp/pull/817/files#diff-df12f6cc542a582b603c601e5ecc0a28dc39259b8484f153ee2f181800ebd389

Comment on lines 23 to 24
propagatedBuildInputs =
[ mathcomp.algebra bignums paramcoq multinomials mathcomp-real-closed ];
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
propagatedBuildInputs =
[ mathcomp.algebra bignums paramcoq multinomials mathcomp-real-closed ];
propagatedBuildInputs = [ mathcomp.algebra bignums paramcoq multinomials mathcomp-real-closed ];

Please use nixpkgs-fmt instead of nix-fmt.

Copy link
Member

Choose a reason for hiding this comment

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

@SuperSandro2000 I'm very much in favor of using an auto-formatter, but could you make sure that this recommendation is documented in the contributing guidelines? For now, there is nothing on Nix formatting in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md nor in https://nixos.org/manual/nixpkgs/unstable/#idm140737315817472.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, done (this changed quite a bit more though)

In order to include matrix normal forms in
CoqEAL (coq-community/coqeal#54)
we add a dependency to mathcomp-real-closed.
@proux01
Copy link
Contributor Author

proux01 commented Oct 26, 2021

Thanks @Zimmi48 for the explanation, done

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Oct 27, 2021
@vbgl
Copy link
Contributor

vbgl commented Oct 31, 2021

I believe that dropping the second commit will solve the conflicts…

@proux01
Copy link
Contributor Author

proux01 commented Nov 2, 2021

Let's try that

@vbgl vbgl merged commit 3e66c40 into NixOS:master Nov 2, 2021
@proux01 proux01 deleted the coqeal-add-master-dep branch November 2, 2021 15:28
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants