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

cubical-mini: init at nightly-20241214 #365340

Merged
merged 2 commits into from
Dec 19, 2024
Merged

cubical-mini: init at nightly-20241214 #365340

merged 2 commits into from
Dec 19, 2024

Conversation

thelissimus
Copy link
Contributor

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: agda "A dependently typed programming language / interactive theorem prover" 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels Dec 15, 2024
@NixOSInfra NixOSInfra added the 12. first-time contribution This PR is the author's first one; please be gentle! label Dec 15, 2024
@thelissimus thelissimus force-pushed the init-agda-cubical-mini branch from ad6db6b to 6684f41 Compare December 15, 2024 11:23
@ofborg ofborg bot added 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Dec 15, 2024
steeleduncan
steeleduncan suggested changes Dec 15, 2024
View reviewed changes
pkgs/development/libraries/agda/cubical-mini/default.nix Show resolved Hide resolved
pkgs/development/libraries/agda/cubical-mini/default.nix
];

buildPhase = ''
export HOME=$TMP
Copy link
Contributor

Choose a reason for hiding this comment

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

runHook preBuild before and runHook postBuild after

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@ncfavier ncfavier Dec 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The point of preBuild is generally to allow users to run commands before the buildPhase. If we use it ourselves, then if a user tries to override it the package will not build any more. We should just move these commands to buildPhase, between runHook preBuild and make.

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe add a comment explaining why this dance is needed (the Makefile uses cabal run ..., and the default Cabal config makes it require an Internet connection).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed and pushed.

Copy link
Member

Choose a reason for hiding this comment

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

Also maybe add a comment explaining why this dance is needed (the Makefile uses cabal run ..., and the default Cabal config makes it require an Internet connection).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I didn't see your second comment the first time, because I didn't refresh the GitHub page. One moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the comment too.

pkgs/development/libraries/agda/cubical-mini/default.nix Outdated Show resolved Hide resolved
pkgs/development/libraries/agda/cubical-mini/default.nix Show resolved Hide resolved
@thelissimus thelissimus force-pushed the init-agda-cubical-mini branch from 6684f41 to 65f0f0d Compare December 16, 2024 11:13
steeleduncan
steeleduncan reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@steeleduncan steeleduncan left a comment

Choose a reason for hiding this comment

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

I don't have commit access, and don't maintain a similar package, so it doesn't make sense for me to leave an approving review. However I wouldn't want to block the PR in any way

My suggestion now would be to find someone who has merged a similar package into nixpkgs recently, and request a review from them

@thelissimus
Copy link
Contributor Author

@ncfavier I noticed that you recently merged a PR for agda-stdlib (#339651). Would you mind taking a look at this PR and merge it if everything looks good?

@thelissimus thelissimus force-pushed the init-agda-cubical-mini branch from 65f0f0d to 85d5f4e Compare December 19, 2024 17:20
@thelissimus thelissimus force-pushed the init-agda-cubical-mini branch from 85d5f4e to 602677c Compare December 19, 2024 17:57
ncfavier
ncfavier approved these changes Dec 19, 2024
View reviewed changes
Copy link
Member

@ncfavier ncfavier left a comment

Choose a reason for hiding this comment

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

Thanks!

@ncfavier ncfavier merged commit df2de39 into NixOS:master Dec 19, 2024
21 of 22 checks passed
@thelissimus thelissimus mentioned this pull request Jan 15, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@steeleduncan steeleduncan steeleduncan requested changes

@ncfavier ncfavier ncfavier approved these changes

Assignees
No one assigned
Labels
6.topic: agda "A dependently typed programming language / interactive theorem prover" 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@thelissimus @steeleduncan @ncfavier @NixOSInfra