build: run diff-hook under --check and document diff-hook #2798
Conversation
| <para>The diff hook can be run as root. Take care to run as little | ||
| as possible as root, for this example we use <command>runuser</command> | ||
| to drop privileges. | ||
| </para> |
There was a problem hiding this comment.
Hm, rather than document the lack of security, we should just fix? E.g. run the hook under the build user ID?
There was a problem hiding this comment.
Good idea, I updated the docs and it now runs as the build user / build group.
|
I want to emphasize that I don't write much C++, so please do carefully review this :P That said, it does work locally! :) |
src/libstore/build.cc
Outdated
| if (diff != "") | ||
| printError(chomp(diff)); | ||
| } catch (Error & error) { | ||
| printError("diff hook execution failed: %s", error.what()); |
There was a problem hiding this comment.
Returning the exit code from the diff-hook might be a nice alternative for #2779
There was a problem hiding this comment.
mmmm I'd like #2779 better :) as returning this exit code means the diff hooks have to be very smart about their code to avoid being confusing with other Nix exit codes.
|
Related: #2619 I never got |
|
@arianvp this PR doesn't change the handling of the options, although it does try to fix the mystery around why your options weren't working: |
|
Also, if you follow the "tutorial" bit it should work! |
src/libstore/build.cc
Outdated
| throw SysError("setuid failed"); | ||
|
|
||
| try { | ||
| auto diff = runProgram(diffHook, true, {tryA, tryB, drvPath, tmpDir}); |
There was a problem hiding this comment.
To prevent a double fork, it would be nice to move uid-setting into runProgram, i.e. by adding uid/gid options to RunOptions.
There was a problem hiding this comment.
That seems good, should I pass allowVfork in as a RunOption? How should it default?
doc/manual/command-ref/conf-file.xml
Outdated
| </listitem> | ||
| </orderedlist> | ||
|
|
||
| <para>The diff hook should not print data to stderr or stdout, as |
There was a problem hiding this comment.
What exactly do you mean here? I guess it should print to stderr right? Otherwise how does the user figure out the output of the diff hook?
There was a problem hiding this comment.
No. None of the output printed by the diff hook is shown to the user, it all goes to the nix-daemon log.
There was a problem hiding this comment.
But that doesn't mean it is 'bad' for a diff-hook to output something right? The sentence now seems to imply that diff hooks shouldn't print results. But we do want them to print results, such that we can actually see what the diff-hook found out, when looking at the nix-daemon logs.
There was a problem hiding this comment.
Sure, though writing the diff report elsewhere might be a better bet. Do you have a suggestion on exactly how to reword this?
There was a problem hiding this comment.
Note that if the diff hook prints data to stderr or stdout, and nix-daemon is enabled, this is not displayed directly to the user. Instead,
it will be printed in the nix-daemon log
(Assuming here that if you use diff-hook without nix-daemon, it will show up directly to the user)
There was a problem hiding this comment.
I don't remember how this is implemented exactly, but I think it might be pretty easy to forward the output of the hook to the client. Similar to how this is done for build logs.
src/libutil/util.cc
Outdated
| @@ -1025,6 +1026,16 @@ void runProgram2(const RunOptions & options) | |||
| if (source && dup2(in.readSide.get(), STDIN_FILENO) == -1) | |||
| throw SysError("dupping stdin"); | |||
|
|
|||
| //if (options.chdir && chdir((*options.chdir).c_str()) == -1) | |||
| // throw SysError("chdir failed"); | |||
There was a problem hiding this comment.
Was this intended to be commented out?
There was a problem hiding this comment.
Oops ... Commented that out thinking it was causing #2801. I'll uncomment and re-test. Good catch, thank you.
|
Uncommented, tested it, and pushed a fixup commit. I'm thinking it'd be best to merge this as a squashed commit -- no need for individual commits. |
build: run diff-hook under --check and document diff-hook
for r13y.com