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

expose gcov, gcc-ar, gcc-ranlib and others gcc tools in gcc-wrapper (fixes #86272) #88202

Open
wants to merge 1 commit into
base: master
from

Conversation

@sorokin
Copy link
Contributor

sorokin commented May 19, 2020

Motivation for this change

This commit fixes #86272. For full description, see the original bug report. It can be summarized as: previously NixOS didn't provide any gcc tools (like gcov, gcc-ar, gcc-ranlib and others).

Things done

I have tested this changes locally on a slightly outdated version (19.09) and now I cherry-picked them on top of master (there was a conflict). I haven't rebuilt master because compilation on my laptop takes days.

  • 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.
@Ericson2314
Copy link
Member

Ericson2314 commented May 20, 2020

Check out what the bintools wrapper does, which puts the unwrapped stuff on the PATH but with lower priority.

I don't know whether that's any better/worse than this, but it would be nice if they both worked consistently, whatever way we choose.

@sorokin
Copy link
Contributor Author

sorokin commented May 20, 2020

Thank you. I didn't know that there was a similar issue in binutils and it was already solved. I like the solution with priorities more than symlinking, because it doesn't require enumerating all the programs. In theory it may work properly even if new commands are added/removed.

Looks like what is needed is:

  1. Whenever gcc wrapper is installed, install also unwrapped gcc. After quick searching I see this can be done by using nix-support/propagated-user-env-packages.
  2. Somehow assign priorities (meta.priority) so wrapped gcc has higher priority then unwrapped.

I'm afraid I don't quite understand how these priorities work for bintuils. In pkgs/development/tools/misc/binutils/default.nix I see this line:

    /* Give binutils a lower priority than gcc-wrapper to prevent a
       collision due to the ld/as wrappers/symlinks in the latter. */
    priority = 10;

Ok, 10 is priority for unwrapped binutils. But then in pkgs/build-support/bintools-wrapper/default.nix (as I understand wrapper for bintuils) we have this:

  meta =
    let bintools_ = if bintools != null then bintools else {}; in
    (if bintools_ ? meta then removeAttrs bintools.meta ["priority"] else {}) //
    { description =
        stdenv.lib.attrByPath ["meta" "description"] "System binary utilities" bintools_
        + " (wrapper script)";
      priority = 10;
  } // optionalAttrs useMacosReexportHack {
    platforms = stdenv.lib.platforms.darwin;
  };

So the priority is set to 10 too. I think I don't quite understand how wrapped bintutils get higher priority than unwrapped.

@Ericson2314
Copy link
Member

Ericson2314 commented May 20, 2020

Oh, I thought this was about during builds too, but I see with https://github.com/NixOS/nixpkgs/blob/master/pkgs/build-support/cc-wrapper/setup-hook.sh#L91-L94 (and a corresponding line in bintools-wrapper) that's already fixed. Then for consistency we with those we should definitely do it the extra-dep (not extra wrapper) way.

I can't say I understand that thing in the meta either, but maybe just try it? If it works for one it should work for the other, right?

@FRidh FRidh added this to WIP in Staging via automation Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.