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

Extend writeShellScript and friends #123608

Closed
wants to merge 2 commits into from
Closed

Extend writeShellScript and friends #123608

wants to merge 2 commits into from

Conversation

colinxs
Copy link

@colinxs colinxs commented May 19, 2021

Motivation for this change

From this discussion over at home-manager it came up that it would be useful to extend writeShellScript to provide both executable and non-executable variants (writeShellScript and writeShellText, resp.), and to support additional shells (e.g. writeZSHScript and writeZSHText).

The motivation for this change is taken from home-manager where a large number of modules generate shell text (e.g. .bashrc) or scripts. Making sure these files are correct is critical to avoiding a corrupted system (the other day I had to jump into the terminal and debug a broken .profile). These files may or may not be executable and run in a variety different shells. The same issue arises for NixOS modules. We discussed adding these functions in home-manager, but thought nixpkgs would be a better home.

This PR is just a sketch as I wasn't sure how the bootstrapping worked in pkgs/top-level/stage.nix and because there's a significant amount of duplication in its current form.

I figured I'd solicit feedback before cleaning things up.

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.

runtime = {
preamble = "#!${runtimeShell}";
checkPhase = ''
${stdenv.shell} -n $out
Copy link
Member

Choose a reason for hiding this comment

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

I believe you'll want runtimeShell here as well:

Suggested change
${stdenv.shell} -n $out
${runtimeShell} -n $out

Copy link
Author

Choose a reason for hiding this comment

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

yup! That's what I get for writing this in 5 minutes ha.

${stdenv.shell} -n $out
'';
};
bash = {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if not this could be removed, since runtimeShell variable is set to bash already.

Copy link
Author

Choose a reason for hiding this comment

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

It's a bit superfluous, but I liked the distinction between the nixpkgs-wide runtime shell, whatever that may be, and bash. That way if I choose bash, I know it's bash, rather than assuming runtimeShell == bash. I know there was talk awhile ago of switching runtimeShell to dash for performance.

'';
};
zsh = {
preamble = "#!${lib.getBin zsh}/bin/zsh";
Copy link
Member

Choose a reason for hiding this comment

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

Could do

Suggested change
preamble = "#!${lib.getBin zsh}/bin/zsh";
preamble = "#!${lib.getBin zsh}/${zsh.shellPath}";

and same with check path below. Indeed could generalize this by creating a function that takes the shell package as an argument.

Hmm, better yet. Maybe it would be possible to put the write* functions directly as a passthrough in the derivation? It would be pretty cool if could do something like

pkgs.zsh.writeProgram "foo" {} ''
  echo hello world
''

or

pkgs.fish.writeProgram "foo" {} ''
  echo hello world
''

or even

pkgs.ghc.writeProgram "foo" {} ''
  main = putStrLn "hello world"
''

I kind of like that since it feels more modular, we wouldn't have to put so many new functions in trivial-builders. It would also be possible to more naturally use a specific package (e.g., with overrides) in the creation of the program.

Copy link
Member

Choose a reason for hiding this comment

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

Note, my idea with the {} is that it would allow whatever customization that may be necessary, e.g.,

pkgs.zsh.writeProgram "foo" { executable = false; } ''
  echo hello world
''

Copy link
Member

Choose a reason for hiding this comment

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

Created a quick and dirty proof of concept in #124080.

Copy link
Author

Choose a reason for hiding this comment

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

I like it! It definitely makes more sense once you start thinking of writePythonScript, writeJuliaScript, etc.

I've seen more chatter lately of cleaning up nixpkgs in general (e.g. #107539), so maybe this could part of that discussion?

I'd love if there was a standardized interface for where these kinds of functions should live. I kind of like the idea of there being a canonical langPlatform (e.g. rustPlatform) for every language where all things related to lang could live.

@stale
Copy link

stale bot commented Jan 9, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 9, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 12, 2023
@abathur
Copy link
Member

abathur commented Aug 27, 2023

@colinxs are you still interested in pursuing this?

@abathur
Copy link
Member

abathur commented Sep 4, 2023

Tentatively closing. Happy to reopen if you're still interested.

@abathur abathur closed this Sep 4, 2023
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.

4 participants