Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[RFC 0075] Declarative wrappers #75
base: master
Are you sure you want to change the base?
[RFC 0075] Declarative wrappers #75
Changes from 33 commits
ef81400b780229d09a05837c7a8c2a0a630c9d7055abb7609d1cadb6a570b1a8db36055ddd60b778bc9944a3b870b259fad75c73e94038bd0fa64abffacf5ff754ad74ab044860d3825f3416a0c092848732246d67b002bcac0cbb7b0c9e1034a13eedbf31548f911873421fdf0c2533391f5d0fb0dd98c51c9c031a24a0a6123b9a65ac77File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Both of the issues related to gdk-pixbuf only happened for non-wrapped derivations and current
wrapGAppsHookwould already fix them.The issue was, IIRC, that gdk-pixbuf NixOS module sets
GDK_PIXBUF_MODULE_FILEenvironment variable, which was enabling binary gdk-pixbuf modules with ABI incompatible with the gdk-pixbuf the programs linked against. In one case it was caused by a bug that slipped through the ABI stability guarantees, in the other by running a program fori686onx86_64.Either way, it is somewhat orthogonal to wrappers. Only that we might want to always wrap graphical applications with librsvg gdk-pixbuf module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though there is one concern around
GDK_PIXBUF_MODULE_FILE– the environment variable supports only a single path so we need to build the cache containing all the modules usinggdk-pixbuf-query-loaders– the current hook actually does not actually do that! but it is required for some image formats to work in e.g.eog. We will need to take inspiration in the module.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More related issues with wrappers NixOS/nixpkgs#160923 and the ones linked at NixOS/nixpkgs#160923 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative to this is the libtool approach, which still allow you to use strace,gdb etc: https://www.gnu.org/software/libtool/manual/html_node/Invoking-libtool.html
If a binary wrapper is used than it should be compiled for every target as this increases package size/build time. Instead having the wrapper reading its actual configuration from the same directory might work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compiling a binary wrapper for each target is probably a small overhead. All it needs to do is modify the environment and exec the target. This should be a trivial C program. I think I would rather it be something trivial like that then needing to read config files which is somewhat more complicated. Although if we really want to optimize we would need some proper profiling about build time and execution time (including cache effects).
But either way I think this proposal enables us to solve this problem, we can figure out the details when start to tackle it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Mic92 do you mean it should not be compiled for each package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worked a bit on a wrapper tool some time ago: svep. Maybe there are some useful ideas there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a nice tool :), but I'm not sure it helps a lot, it seems it only supplies an alternative to
makeCWrapper, but that depends on rust, and not pure C99.We won't be able for example to use it to save the wrapping information of a package, as we want to access that information independently, from
${pkg}/nix-support/, and not from the binaries that the package may distribute.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "store API" is
Dict[String, List[StorePath]]but the "runtime API" isDict[String, String]which raises a number of questions on how we implement that.:is the most common but there are exceptions.We can also go further. Instead of setting
$PATHto a massive list does it make more sense to build a folder of symlinks and set$PATHto a single element? If so will we special-case some variables or should we use a more detailed schema (as proposed below)?Furthermore does this replace or prepend or append? Replace seems the purest but commands such as
parallel -- command-from-my-pathare not uncommon. In that case there seems to be no good option.So it seems to me that no single set of answers to the above questions will handle the cases we already know about. It seems like we will need to provide more info.
{ "NAME": { "paths": [ ... ], "separator": ":", "sort": "topological", } }Although I think that is probably going to lead to huge complexity (for example do we need escaping for any var?) It probably makes more sense to have more semantic options.
{ "NAME": { "$type": "search-path", "paths": [ ... ], } }Then we can define types that work for
PATH,LUA_PATHand so on. Furthermore this leaves space for "arguments" to these types such as escaping modes or whether to allow/deny/force making a directory of symlinks rather than a large variable. (These don't need to be implemented upfront)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about answers to some of these questions back when I implemented https://github.com/NixOS/nixpkgs/pull/85103/files#diff-aad93ba22adfe208c7b970a7b9d0f73dba9306b8a4fc4eaacfb2ac331aed9ea1R51-R74 . I prepared some answers in the RFC to be pushed, but I don't know:
postInstallsnippet example below, by manually quoting values in the json file..I thought about this exactly even back when I implemented NixOS/nixpkgs#85103 so it will be possible to avoid what NixOS/nixpkgs#84689 fixed. I addressed this idea in the changes I made to the RFC that I haven't pushed yet.
I'm not sure I understand:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If two packages have a
bin/foothen the order will decide which one is used. For most of thesePATH-like variables the first match is used.It is rare but does happen. For things like
PATHI don't think there is even a way to quote elements. You simply can't have a path that contains:in yourPATH. At the very least it would be nice to raise an error. It would be even better if we can "alias" the path to a safe name or escape the invalid characters (if supported by the variable, which is rare)My point is that the argument to a parallel is a command to run, but that command should be pulled from the PATH of the calling shell, not a PATH that may be set but a wrapper in the parallel derivation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's super rare to have two packages providing the same binary like that, and finding them used together in the wrapping while wishing only one package's
out/binto rule..Also paths escaping is too rare, so I think it'll be best to leave these topics out of scope for the RFC.
As for
parallelcommands. Do you meanparallelinvocations in the builds of derivations? something like:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think collisions are super rare. After all nix has a priority system for some reason. I think we should at least define how this is handled.
As for the escaping I think that it is fine to ignore this for now. But I think it should be mentioned explicitly.
For the "parallel problem" I mean in general. Any time you have a wrapper that both needs commands itself as well as forwards a user's command we have this problem. I think we should discuss how this is handled. Currently in nixpkgs it is very inconsistent.
But the most important point still stands. We need some configuration. If only to specify the separator. So I think that needs to be included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PATH order is support important. Tools like trash, safe-rm and molly-guard heavily rely on it. Also I commonly use a higher priority entry in PATH to create wrappers for none interactive shells. For environment variables which are similar to PATH like NIX_PATH order is also super important and ordering the entries for example alphabetically breaks things.
What is safe to do is deduplication when an entry is encountered the second time and I think this should be done by default to prevent to long environment variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably use
${placeholder "out"}here? Or am I missing something?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of people ask that, see NixOS/nixpkgs#85103 (comment) - I linked that explanation in the comment in the code snippet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to have quoting issues?
Either way would it be easier to pass the JSON in as an env var? Then it can just be passed as an argument? Or just use a well-known env var that can be automatically used by the command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see quoting issues.. We can use:
But that's of course is only a visual change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't
echo "${builtins.toJSON { foo = "bar"; }}"result inecho "{"foo":"bar"}"? Or am I missing something here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I understand now.. Perhaps we should also use
lib.strings.escapeShellArg. I have changed this composition into something much nicer.