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

nix-output-monitor: Add nom-build script #114903

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

maralorn
Copy link
Member

@maralorn maralorn commented Mar 2, 2021

Motivation for this change

By popular demand we present nom-build it's exactly like nix-build but with more nom.

Credits to @lilyball for the idea:
https://discourse.nixos.org/t/announcing-the-nix-output-monitor/11672/6

My bash in nixpkgs foo is not very strong, so I appreciate any style critique.

\cc @mweinelt

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@maralorn
Copy link
Member Author

maralorn commented Mar 2, 2021

I am torn on the question if this is something I should put in the upstream repository. But patching the paths will be at least the same number of lines here in post-install, so I don‘t see a big benefit.

@r-rmcgibbo
Copy link

r-rmcgibbo commented Mar 2, 2021

Result of nixpkgs-review pr 114903 at 632be598 run on aarch64-linux 1

2 packages built:

Result of nixpkgs-review pr 114903 at 632be598 run on x86_64-linux 1

2 packages built:

@@ -25,6 +26,10 @@ mkDerivation {
ansi-terminal async attoparsec base containers directory HUnit mtl
nix-derivation process relude stm text time unix
];
postInstall = ''
echo -e "#!/bin/sh\n${expect}/bin/unbuffer nix-build \"\$@\" 2>&1 | exec $out/bin/nom" > $out/bin/nom-build
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
echo -e "#!/bin/sh\n${expect}/bin/unbuffer nix-build \"\$@\" 2>&1 | exec $out/bin/nom" > $out/bin/nom-build
echo -e "#!/bin/sh
${expect}/bin/unbuffer nix-build \"\$@\" |& $out/bin/nom
" > $out/bin/nom-build

Why do we want exec here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idk, I don‘t know of a disadvantage and I don‘t see that we need the running bash process after nom has been started. Also maybe exit code propagation?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exec is really just to get rid of the bash process, there’s no reason to keep it around. It also will keep the ps output cleaner.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would probably also be a bit cleaner if written using a here-doc. At a bare minimum, I don’t see why this is using the -e flag to echo. And the shebang should probably be #!${runtimeShell}.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heck, you could probably make this a writeShellScript instead, using the outPath attribute of the resulting derivation, like

let
  nom-build = writeShellScript "nom-build" ''
    ${expect}/bin/unbuffer nix-build "$@" |& ${self}/bin/nom
  '';
  self = mkDerivation rec {
    # …
    postInstall = ''
      install -Dm755 ${nom-build} $out/bin/nom-build
    '';
  };
in self

I haven’t tried this directly myself, but I think the outPath is available without forcing anything beyond name and fixed-hash attrs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If thought about that, but was un-optimistic. I’ll try.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that doesn‘t make sense. If the hash of nom changed the hash of nom-build needs to change and vice versa. I‘ll try anyways just to make sure, but I would be thoroughly surprised.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback, I have rewritten the function with your hints. Looks much better now.

@SuperSandro2000
Copy link
Member

Now we need bash-completion for nom-build.

@maralorn
Copy link
Member Author

maralorn commented Mar 3, 2021

Now we need bash-completion for nom-build.

Fair enough. But I have no clue how to do that …

@SuperSandro2000
Copy link
Member

Now we need bash-completion for nom-build.

Fair enough. But I have no clue how to do that …

hedning/nix-bash-completions#16

@SuperSandro2000 SuperSandro2000 merged commit c872f28 into NixOS:master Mar 3, 2021
@maralorn maralorn deleted the nom-build branch March 3, 2021 14:56
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/announcing-the-nix-output-monitor/11672/7

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.

6 participants