-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Execline replace bash wrapper #71129
Conversation
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.
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>
Can we make the non-wrapped version of |
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 don't particularly like the idea of having `grep` run on every
invocation by default, it seems more like something someone should
opt-into.
I agree with this. I propose we just compile a little program like
this (untested):
#include <stdlib.h>
#include <string.h>
#include <assert.h>
#include <stdio.h>
#include <unistd.h>
int main(int argc, char *const argv[])
{
char *path = getenv("PATH");
char *sep = path ? ":" : "";
if (!path) path = "";
size_t size = strlen(BIN_PATH) + strlen(sep) + strlen(path) + 1;
char *new_path = calloc(size, sizeof(char));
if (!new_path)
goto err;
assert(sprintf("%s%s%s", BIN_PATH, sep, path));
execv(BIN_PATH "/execline", argv);
err:
fprintf(stderr, "execline (nixpkgs): %s", strerror(errno));
return 1;
}
Avoids doing any more work than absolutely necessary, and should work on
macOS.
|
Indeed, I would prefer a wrapper program that just unconditionally added the path as above (though, I assume you missed a It mimics what the bash wrapper did anyway. (actually, I'm always slightly annoyed that nixpkgs |
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 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 |
I’ll merge this for now, we should tackle the C wrapper soon. |
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.