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

Revert "stdenv: log build hooks as they run" #294625

Merged
1 commit merged into from
Mar 10, 2024
Merged

Conversation

ghost
Copy link

@ghost ghost commented Mar 10, 2024

Reverts #290081

  • the hooks are writing to stdout which will break pipelines that use nix-shell as the interpreter
  • default logging is very verbose when using nix-shell

reverting for now -- hopefully this change finds its way back soon

@Qyriad @philiptaron @RaitoBezarius

@ghost ghost requested a review from Ericson2314 as a code owner March 10, 2024 01:36
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: stdenv Standard environment labels Mar 10, 2024
@lf-
Copy link
Member

lf- commented Mar 10, 2024

Please consider instead: nix-shell --no-build-output.

@ghost
Copy link
Author

ghost commented Mar 10, 2024

Please consider instead: nix-shell --no-build-output.

https://nixos.org/manual/nix/stable/command-ref/nix-shell.html#use-as-a--interpreter suggests

#! /usr/bin/env nix-shell
#! nix-shell -i real-interpreter --packages packages

so --no-build-output doesn't seem like a viable workaround, unfortunately.

@ghost ghost merged commit 27a5238 into staging Mar 10, 2024
29 checks passed
@ghost ghost deleted the revert-290081-diag/log-hooks branch March 10, 2024 09:48
@lf-
Copy link
Member

lf- commented Mar 10, 2024

Ah, yeah sorry I realized later I misunderstood the issue. Regressing nix-shell -p is definitely a bit of an ouchie.

There's sneaky stuff that can be done to the logs that can help though, we'll see how that shakes out in the next iteration.

@philiptaron
Copy link
Contributor

Yeah, I'm sad this is getting reverted, though I understand the reason why. @Qyriad, if you want to, we can work together to make the next version. 💪🏻

@ghost
Copy link
Author

ghost commented Mar 11, 2024

agree. this is a good change and should make its way back in. looking at it yesterday, perhaps just logging to stderr when NIX_DEBUG is > 0 is sufficient.

@lf-
Copy link
Member

lf- commented Mar 11, 2024

agree. this is a good change and should make its way back in. looking at it yesterday, perhaps just logging to stderr when NIX_DEBUG is > 0 is sufficient.

we actually noticed there's extra message types we can send to NIX_LOG_FD, which will ostensibly appear on higher verbosity levels of nix itself, get saved properly, as well as not invalidate derivations when changed. we've not tried it yet but we did find the nix-side code for such a thing. this would probably be the ideal situation, to decouple print verbosity from build configuration.

@Qyriad
Copy link
Member

Qyriad commented Mar 11, 2024

I started to experiment with it briefly today and it seems to be a viable route so far.

This pull request was closed.
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.

3 participants