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

Execline replace bash wrapper #71129

Merged
merged 2 commits into from Oct 18, 2019

Conversation

Profpatsch
Copy link
Member

Followup to the issue raised in b64d25c#commitcomment-35482293 cc @luke-clifton.

It solves the problem of having the intermediate script invoke bash, just to execute execline.

This does not solve the macOS problem with nested shebangs, but it provides an option to disable the execlineb wrapper (execline.override { execlineb-with-builtins = false; }) to work around that in the meantime.

Sorry for the crappy diff, it’s mainly because I indented the original expression; the original derivation is unchanged.

Tested the resulting execlineb wrapper manually.

Using wrapProgram adds a call to `bash` around every call
of `execline`, which clearly misses the basic idea behind
`execline` in the first place …

This reverts commit b64d25c.
Introduces the `execlineb-with-builtins` flag, which when
true (default) will add all execline builtins to the PATH of
`execlineb`.

This is usually what users expect.

If the flag is set to `false`, the unpatched execline derivation is
returned instead.
Profpatsch referenced this pull request Oct 14, 2019
The execlineb program is the launcher (and lexer) of execline scripts.
So it makes a lot of sense to have all the small tools in scope by
default.
We append to the end of PATH so that they can be easily overwritten by
the user.

Co-authored-by: Alyssa Ross <hi@alyssa.is>
@luke-clifton
Copy link
Contributor

Can we make the non-wrapped version of execlineb the default? s6-rc and most other execline scripts generated by skaware packages seem to use absolute paths for the builtins anyway. I don't particularly like the idea of having grep run on every invocation by default, it seems more like something someone should opt-into.

@alyssais
Copy link
Member

alyssais commented Oct 15, 2019 via email

@luke-clifton
Copy link
Contributor

Indeed, I would prefer a wrapper program that just unconditionally added the path as above (though, I assume you missed a setenv() there).

It mimics what the bash wrapper did anyway. (actually, I'm always slightly annoyed that nixpkgs makeWrapper doesn't create little C programs like this instead of bash scripts)

@Profpatsch
Copy link
Member Author

Can we make the non-wrapped version of execlineb the default? s6-rc and most other execline scripts generated by skaware packages seem to use absolute paths for the builtins anyway.
I don't think this is the primary use case for execline.

I think it’s quite natural for people to expect that execline can access its “stdlib”, similar to how bash can access its builtins.

How it does that exactly I don’t particularly care about (as long as we are not patching the execline source code).

If you need to optimize for speed (e.g. on small devices), you can always use the non-wrapped version. But not wrapping by default is premature optimization in my opinion.

I don't particularly like the idea of having grep run on every invocation by default, it seems more like something someone should opt-into.

Indeed, I would prefer a wrapper program that just unconditionally added the path as above (though, I assume you missed a setenv() there).

I had that before, that doesn’t work for use-cases with a lot of nested execline script (which the main use-case for me, I don’t use it for s6 scripts at the moment).

Feel free to write a C wrapper that does the substring match instead of invoking grep, I’d happily merge that.

@Profpatsch Profpatsch merged commit 2d28753 into NixOS:master Oct 18, 2019
@Profpatsch
Copy link
Member Author

I’ll merge this for now, we should tackle the C wrapper soon.

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.

None yet

3 participants