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

writeShellApplication instead of writeShellScriptBin? #144

Closed
n8henrie opened this issue Feb 7, 2023 · 3 comments
Closed

writeShellApplication instead of writeShellScriptBin? #144

n8henrie opened this issue Feb 7, 2023 · 3 comments

Comments

@n8henrie
Copy link
Collaborator

n8henrie commented Feb 7, 2023

Would you consider a PR that replaces writeShellScriptBin with writeShellApplication?

For a security-focused package, having all the shell code automatically shellchecked might help eliminate potential bugs.

If not a good idea, I'd love to learn why (it certainly seems that writeShellScriptBin is more common in the wild).

@ryantm
Copy link
Owner

ryantm commented Feb 7, 2023

Yes. I have a pending PR that shellchecks it. #139

@n8henrie
Copy link
Collaborator Author

Glad to see it!

The substituteAll approach seems commonplace, but to a nix newbie certainly looks kludgy at first glance, and I don't really understand why writeShellApplication is not more common. As I'm sure you know it does little more than set some bash "safe-mode" settings (that agenix is already using, so could be removed), run shellcheck automatically (obviating the need for your effort that commit), and provide some PATH helpers (which would avoid the need to manually specify e.g. sedBin vs just sed).

It looks like you have over 4k commits to nixpkgs, so I'm sure none of this is news to you, and I'm not asking out of criticism just out of curiosity -- is there a reason you chose / people seem to use writeShellScriptBin over writeShellApplication?

@ryantm
Copy link
Owner

ryantm commented Feb 11, 2023

is there a reason you chose / people seem to use writeShellScriptBin over writeShellApplication?

It might be there mostly for backward compatible reasons.

4k commits to nixpkgs

A lot of my commits are automated version bumps before I moved those over to @r-ryantm, but yeah, I've been around the block a few times :)

I've merged #139 so I'm going to close this. I wrote some reasons why I choose to use mkDerivation there.

@ryantm ryantm closed this as completed Feb 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants