Skip to content

checkpointBuildTools: mkCheckpointedBuild -> mkCheckpointBuild#279487

Merged
roberth merged 3 commits intoNixOS:masterfrom
bryango:checkpoint-build-polish
Jan 9, 2024
Merged

checkpointBuildTools: mkCheckpointedBuild -> mkCheckpointBuild#279487
roberth merged 3 commits intoNixOS:masterfrom
bryango:checkpoint-build-polish

Conversation

@bryango
Copy link
Member

@bryango bryango commented Jan 8, 2024

Description of changes

This PR follows from #167670 (comment) @messemar.

All other functions in checkpointBuildTools are in the form of *CheckpointBuild* without the ed, so we trade the mkCheckpointedBuild name in favor of mkCheckpointBuild for a uniformed look (and link mkCheckpointedBuild to mkCheckpointBuild with a lib.warn "deprecated", although currently there is no such usage yet in nixpkgs).

Also, apply suggestions from @phip1611 and @infinisil that did not make into the last PR.

Note: #167670 (comment) actually does not work: the sourceDifference.patch file gets deleted by rm -r * in the subshell (yeah shell is weird and surprising, as usual!). Instead, we use mktemp to create a temporary file to store the patch. The downside of this is that the patch size is limited by the size of /tmp.

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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 the 8.has: documentation This PR adds or changes documentation label Jan 8, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Jan 8, 2024
@bryango bryango force-pushed the checkpoint-build-polish branch from 4b1bea7 to 285ac09 Compare January 8, 2024 10:19
@bryango bryango marked this pull request as ready for review January 8, 2024 10:24
@bryango bryango requested a review from roberth January 8, 2024 10:24
@messemar
Copy link
Contributor

messemar commented Jan 8, 2024

@bryango

Note: #167670 (comment) actually does not work: the sourceDifference.patch file gets deleted by rm -r * in the subshell (yeah shell is weird and surprising, as usual!). Instead, we use mktemp to create a temporary file to store the patch. The downside of this is that the patch size is limited by the size of /tmp.

If the subshell approach does not work, we should keep the previous approach.
Otherwise we introduce a point of failure which is hard to debug actually.

@roberth
Copy link
Member

roberth commented Jan 8, 2024

@ofborg build tests.checkpoint-build

Btw maybe that should become tests.checkpointBuildTools

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

I have no opinion about the copying related change - as long as it works and that's "proven" by the tests.

@bryango
Copy link
Member Author

bryango commented Jan 8, 2024

Note: #167670 (comment) actually does not work: the sourceDifference.patch file gets deleted by rm -r * in the subshell (yeah shell is weird and surprising, as usual!). Instead, we use mktemp to create a temporary file to store the patch. The downside of this is that the patch size is limited by the size of /tmp.

If the subshell approach does not work, we should keep the previous approach. Otherwise we introduce a point of failure which is hard to debug actually.

The current solution is almost identical to the previous implementation, but did solve the issue raised by #167670 (comment). I think it would be good to keep it this way.

All other functions are in the form of `*{c,C}heckpointBuild*`, so we
deprecate the `mkCheckpointedBuild` function in favor of `mkCheckpointBuild`.

Also address some inconsistencies in the docs: some `buildOutput` should
actually be `incrementalBuildArtifacts`.
@bryango bryango force-pushed the checkpoint-build-polish branch from 4cfec3b to f73f15f Compare January 8, 2024 11:51
@bryango
Copy link
Member Author

bryango commented Jan 8, 2024

ofborg build tests.checkpoint-build

Btw maybe that should become tests.checkpointBuildTools

The test rename is implemented! Waiting for ofborg, I think it knows how to find the new test! Update: no it does not, ofborg manually summoned.

@ofborg ofborg bot added 8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package labels Jan 8, 2024
@bryango bryango requested a review from roberth January 8, 2024 12:32
@bryango
Copy link
Member Author

bryango commented Jan 8, 2024

@ofborg build tests.checkpointBuildTools

Add some minor improvements of the package, and use temp files for the
source difference patch, such that it is more reliable. This follows
from the suggestions of @infinisil.

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
@bryango bryango force-pushed the checkpoint-build-polish branch from f73f15f to 9ebdc06 Compare January 8, 2024 17:16
@bryango bryango requested a review from infinisil January 8, 2024 17:22
@bryango bryango force-pushed the checkpoint-build-polish branch from 9ebdc06 to d7253be Compare January 8, 2024 17:34
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

(CI failure is unrelated)

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Jan 8, 2024
@bryango
Copy link
Member Author

bryango commented Jan 9, 2024

Update: CI all green now!

@bryango
Copy link
Member Author

bryango commented Jan 9, 2024

I believe this is ready to be merged now! If there is anything else that you would like me to improve, please don't hesitate to ping me at any time!

@roberth roberth merged commit 098ffee into NixOS:master Jan 9, 2024
@delroth delroth added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Jan 9, 2024
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-279487-to-release-23.11 origin/release-23.11
cd .worktree/backport-279487-to-release-23.11
git switch --create backport-279487-to-release-23.11
git cherry-pick -x df62c3c87f35cd50d6ee20ea92900bb03ad827c9 5f3aad00ffc0a769a1ccc5b3b521221aa5e5d809 d7253bea6d7366987acce31c2c2355ffbdf389b4

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Jan 9, 2024

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-279487-to-release-23.11 origin/release-23.11
cd .worktree/backport-279487-to-release-23.11
git switch --create backport-279487-to-release-23.11
git cherry-pick -x df62c3c87f35cd50d6ee20ea92900bb03ad827c9 5f3aad00ffc0a769a1ccc5b3b521221aa5e5d809 d7253bea6d7366987acce31c2c2355ffbdf389b4

@roberth
Copy link
Member

roberth commented Jan 9, 2024

Thank you @bryango!

});

mkCheckpointedBuild = lib.warn
"`mkCheckpointedBuild` is deprecated, use `mkCheckpointBuild` instead!"
Copy link
Member Author

@bryango bryango Jan 11, 2024

Choose a reason for hiding this comment

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

[grammar] Remember to port this:

From a3b90e768b8d43e0efb9cc227153d55609c65999 Mon Sep 17 00:00:00 2001
From: bryango <bryango@users.noreply.github.com>
Date: Thu, 11 Jan 2024 22:52:52 +0800
Subject: [PATCH] checkpointBuildTools.mkCheckpointedBuild: update deprecation
 warning

Co-authored-by: Robert Hensing <roberth@users.noreply.github.com>
---
 pkgs/build-support/checkpoint-build.nix | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pkgs/build-support/checkpoint-build.nix b/pkgs/build-support/checkpoint-build.nix
index c9bee45005a13a..cc70eddd7ba303 100644
--- a/pkgs/build-support/checkpoint-build.nix
+++ b/pkgs/build-support/checkpoint-build.nix
@@ -85,6 +85,6 @@ rec {
   });
 
   mkCheckpointedBuild = lib.warn
-    "`mkCheckpointedBuild` is deprecated, use `mkCheckpointBuild` instead!"
+    "`mkCheckpointedBuild` is deprecated; use `mkCheckpointBuild` instead."
     mkCheckpointBuild;
 }

... in some future revision.

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

Labels

8.has: clean-up This PR removes packages or removes other cruft 8.has: documentation This PR adds or changes documentation 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants