writeShellApplication: Document and extend arguments #280592

9999years · 2024-01-12T21:48:02Z

Description of changes

Documented pkgs.writeShellApplication arguments, expanded the test suite, and added several new arguments:

The default Bash options can be customized with bashOptions.

This is useful to e.g. exclude nounset if you want to check [[ -n "$myVar" ]] on a potentially-unset variable, or disable pipefail to use grep in a pipeline. Previously you could do set +o pipefail, but running that right after set -o pipefail always felt clumsy to me.
Added a derivationArgs argument which is used for extra stdenv.mkDerivation arguments.

This allows customizing postCheck and other such parameters.
Runtime environment variables can now be set with runtimeEnv.

I also took care to make sure this doesn't change the hashes of derivations produced with any of the involved functions, so we don't have to worry about excessive rebuilds.

Things done

Add a 👍 reaction to pull requests you find important.

infinisil

The new tests are awesome, thanks!

I have some comments for the other bits though :)

infinisil · 2024-01-15T21:29:36Z

pkgs/build-support/trivial-builders/default.nix

@@ -152,19 +152,30 @@ rec {
    , meta ? { }
    , allowSubstitutes ? false
    , preferLocalBuild ? true
-    }:
+    , ...
+    }@args:


runCommandWith already supports the derivationArgs attribute with the same purpose (which I also think is cleaner). The same interface can be adopted for writeTextFile and writeShellApplication.

writeTextFile passes all of its arguments to runCommandWith as derivationArgs. I don't think this makes sense unless you also support renaming writeTextFile's checkPhase argument to derivationArgs.checkPhase.

In the sense that writeShellApplication wraps writeTextFile and writeTextFile wraps runCommand, this is just extending the existing behavior to forward arbitrary arguments in addition to the explicitly blessed ones.

The ... pattern where you pick certain arguments and forward the rest to some mkDerivation-derivative is just a bit too full of holes imo:

It makes typos not trigger an error, since mkDerivation happily accepts pretty much any value and uses them as environment variables.

It makes it so that you can't use new mkDerivation attributes in case they happen to conflict with the picked arguments (what if mkDerivation gets a destination attribute with different semantics?).

And similarly, if somebody passes some derivation attribute, but later that becomes a picked-out function argument, stuff might break.

It makes the distinction between the stable function interface and internals less clear. In particular setting derivation attributes has the potential to break stuff, whereas the function arguments don't.

It makes the code less maintainable and harder to understand. Updating function arguments now requires changes in two parts of the code and some thinking. Solutions have been proposed to this, but I think we should just design better interfaces instead

Should I change writeTextFile's interface as well, to be consistent?

I think so yeah

OK, though I suspect this will cause breakage with callers and confusion around arguments like meta and checkPhase which are also stdenv.mkDerivation arguments.

pkgs/build-support/trivial-builders/default.nix

infinisil · 2024-01-15T21:41:57Z

pkgs/build-support/trivial-builders/default.nix

@@ -367,6 +367,7 @@ rec {
    , text
    , runtimeShell ? runtimeShell'
    , runtimeInputs ? [ ]
+    , runtimeEnv ? null


This reminds me of makeWrapper, which provides the same functionality via --set <VAR> <VALUE>. How about we make this function internally use makeWrapper instead? We can then also use --prefix PATH : ${makeBinPath runtimeInputs}.

makeWrapper doesn't support setting values to lists or maps. I also think it's nicer to keep the written script contained to one file.

Fair enough, we might want to reconsider this in the future though, especially if the need for more makeWrapper-like features comes up.

infinisil · 2024-01-15T21:44:37Z

pkgs/build-support/trivial-builders/default.nix

+      /* The name of the script to write.
+
+         Type: String
+       */


Unfortunately the doc comments here aren't rendered into the manual. Can you follow the style from the very new #277534 instead?

Oh they aren't? But we could wire them up, no?

Yeah that could be done with nixdoc, just like with the lib docs.

fricklerhandwerk

That's a great improvement, thanks a lot. Finally we can configure the shell!

fricklerhandwerk · 2024-01-15T21:51:38Z

pkgs/build-support/trivial-builders/default.nix

-    Note that the checkPhase uses stdenv.shell for the test run of the script,
-    while the generated shebang uses runtimeShell. If, for whatever reason,
-    those were to mismatch you might lose fidelity in the default checks.
+    Similar to `writeShellScriptBin` and `writeScriptBin`.


Maybe better:

Suggested change

Similar to `writeShellScriptBin` and `writeScriptBin`.

Similar to [](#trivial-builder-writeShellScriptBin) and [](#trivial-builder-writeScriptBin).

This will render sensibly in HTML now that #277534 is merged.

This kind of directly contradicts what I said in #280592 (comment) 😅

I guess you meant that if this content is moved to trivial-build-helpers.chapter.md, it would get rendered correctly.

OK, updated the manual. I'll leave the argument documentation where it is, so it'll get rendered when nixdoc gets wired up.

I guess it's good to have docs at all, so I don't want to block on this, but really it would be best if the docs were actually visible from the manual instead of people having to look through the source all the time.

Also I want to highlight that @DanielSidhion is recently writing some really nice rendered documentation for function arguments:

nixpkgs/doc/build-helpers/images/dockertools.section.md

Line 28 in 7d021ca

### Inputs {#ssec-pkgs-dockerTools-buildImage-inputs}

Rendered: https://nixos.org/manual/nixpkgs/unstable/#ssec-pkgs-dockerTools-buildImage-inputs

pkgs/build-support/trivial-builders/default.nix

9999years · 2024-01-29T18:31:34Z

Addressed review comments, PTAL.

infinisil

Whoops didn't want to force push here yet before I got confirmation first (asked you in PMs), I undid that for now. Mainly I'm suggesting to clean up the Git log, but also to split the bash stuff into a separate PR, because it's a bit trickier to get right.

I made the changes in this branch, the diff to your current branch is here. Let me know if it sounds good to push like that.

infinisil · 2024-02-01T22:14:27Z

pkgs/build-support/trivial-builders/default.nix

-          ${stdenv.shellDryRun} "$target"
+          # This is `shellDryRun` but for our particular `bash`.
+          ${bash} -n -O extglob "$target"


stdenv.shell (or stdenv.shellDryRun) and pkgs.runtimeShell are not the same, the former is for the build platform, while the latter is for the host platform. This means that replacing both with the same version breaks cross compilation, and I don't see an obvious way to get around this. That's what I meant with splicing shenanigans being necessary in this comment.

I don't know an obvious way to make this work in a clean way, so I'd say let's have that in a separate PR.

infinisil

I see you pushed my suggested changes, so this definitely looks good to me! Let's merge when CI is greenish and improve anything as necessary going forward.

9999years force-pushed the write-shell-application-options branch 4 times, most recently from 4a1f0d8 to 3047769 Compare January 13, 2024 00:45

9999years marked this pull request as ready for review January 13, 2024 00:47

9999years force-pushed the write-shell-application-options branch from 3047769 to a231103 Compare January 13, 2024 00:51

9999years requested review from fricklerhandwerk and infinisil January 13, 2024 00:52

9999years changed the title ~~writeShellApplication: Document and extend options~~ writeShellApplication: Document and extend arguments Jan 13, 2024

9999years force-pushed the write-shell-application-options branch from a231103 to aaf17c8 Compare January 13, 2024 01:03

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 13, 2024

infinisil requested changes Jan 15, 2024

View reviewed changes

fricklerhandwerk approved these changes Jan 15, 2024

View reviewed changes

delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 15, 2024

9999years force-pushed the write-shell-application-options branch from aaf17c8 to fe2c1bc Compare January 22, 2024 17:33

github-actions bot added the 8.has: documentation label Jan 22, 2024

9999years requested review from fricklerhandwerk and infinisil January 22, 2024 17:34

delroth removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 22, 2024

9999years force-pushed the write-shell-application-options branch 2 times, most recently from f53b152 to 68d79fa Compare January 29, 2024 18:30

ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1 10.rebuild-linux: 1-10 labels Jan 29, 2024

infinisil force-pushed the write-shell-application-options branch from 68d79fa to 2e2b0f0 Compare February 1, 2024 22:00

infinisil force-pushed the write-shell-application-options branch 3 times, most recently from 2188fe9 to 68d79fa Compare February 1, 2024 22:08

infinisil reviewed Feb 1, 2024

View reviewed changes

9999years added 6 commits February 1, 2024 16:02

writeShellApplication: Add bashOptions argument

ac20bcf

writeShellApplication: Add runtimeEnv argument

a647669

writeTextFile,writeShellApplication: Allow setting extra arguments

863786b

writeShellApplication: Document arguments

63f7c9b

writeShellApplication: Expand test suite

88ce0b0

writeShellApplication: Update manual

41376dd

9999years force-pushed the write-shell-application-options branch from 68d79fa to 41376dd Compare February 2, 2024 00:02

infinisil approved these changes Feb 2, 2024

View reviewed changes

ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 and removed 10.rebuild-darwin: 1-10 10.rebuild-linux: 11-100 labels Feb 2, 2024

infinisil merged commit 3c29f55 into NixOS:master Feb 2, 2024
25 checks passed

9999years deleted the write-shell-application-options branch February 2, 2024 17:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

writeShellApplication: Document and extend arguments #280592

writeShellApplication: Document and extend arguments #280592

9999years commented Jan 12, 2024 •

edited

Loading

infinisil left a comment

infinisil Jan 15, 2024

9999years Jan 22, 2024

infinisil Jan 27, 2024

9999years Jan 29, 2024

infinisil Jan 29, 2024

9999years Jan 29, 2024

infinisil Jan 15, 2024

9999years Jan 22, 2024

infinisil Jan 27, 2024

infinisil Jan 15, 2024

fricklerhandwerk Jan 15, 2024

infinisil Jan 15, 2024

fricklerhandwerk left a comment

fricklerhandwerk Jan 15, 2024 •

edited

Loading

infinisil Jan 15, 2024

9999years Jan 22, 2024

infinisil Jan 27, 2024

9999years commented Jan 29, 2024

infinisil left a comment •

edited

Loading

infinisil Feb 1, 2024

infinisil left a comment

	Similar to `writeShellScriptBin` and `writeScriptBin`.
	Similar to [](#trivial-builder-writeShellScriptBin) and [](#trivial-builder-writeScriptBin).

writeShellApplication: Document and extend arguments #280592

writeShellApplication: Document and extend arguments #280592

Conversation

9999years commented Jan 12, 2024 • edited Loading

Description of changes

Things done

infinisil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fricklerhandwerk left a comment

Choose a reason for hiding this comment

fricklerhandwerk Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

9999years commented Jan 29, 2024

infinisil left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

infinisil left a comment

Choose a reason for hiding this comment

9999years commented Jan 12, 2024 •

edited

Loading

fricklerhandwerk Jan 15, 2024 •

edited

Loading

infinisil left a comment •

edited

Loading