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

Refactor gccForLibs so gcc isn't inadvertently compiled #100686

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@spease
Copy link
Contributor

@spease spease commented Oct 16, 2020

Motivation for this change

(I believe) gccForLibs was written in such a way that it would pull in GCC on darwin. This change is from a few days ago, and I'm relatively new to nix.

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.
@spease spease requested a review from matthewbauer as a code owner Oct 16, 2020
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Please fix the eval error. Maybe a rebase helps?

@spease
Copy link
Contributor Author

@spease spease commented Nov 29, 2020

I’m not doing any more nix work on my work machine (which is presently the only active Mac computer I have).

The team I work with decided to stop using nix due to the difficulty of learning it and broken packages (primarily halide and nix-xcodeenv) I encountered with the proof of concept. Two people also heard a horror story and negative feedback from their networks as well that seemed to line up with my experience.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Dec 27, 2020

I guess this can be closed then?

@spease
Copy link
Contributor Author

@spease spease commented Dec 30, 2020

Well, I have a personal mac now, but I don't understand the error. I don't see anywhere that I used anything called restrict-eval?

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Jan 1, 2021

@ofborg eval

# with gcc-7: undefined reference to `__divmoddi4'
if stdenv.targetPlatform.isi686
then gcc6.cc
else if stdenv.targetPlatform == stdenv.hostPlatform && targetPackages.stdenv.cc.isGNU
# Can only do this is in the native case, otherwise we might get infinite
# recursion if `targetPackages.stdenv.cc.cc` itself uses `gccForLibs`.
then targetPackages.stdenv.cc.cc
else gcc.cc;
else gcc.cc
else null;
Copy link
Member

@SuperSandro2000 SuperSandro2000 Jan 1, 2021

I think this is causing the eval error. The package should never be null.

Copy link
Contributor Author

@spease spease Jan 3, 2021

Got it, that's tricky. Would the idiomatic thing be to mark gcc as broken and then check for that rather than its nullness?

Copy link
Member

@SuperSandro2000 SuperSandro2000 Jan 10, 2021

Marking something broken based on targetPlatform sounds weird to me.

Copy link
Contributor Author

@spease spease Jan 12, 2021

Well, as far as I know, that is what's going on - gcc is broken on macOS. Or at least, that's what I thought back in October...

@SuperSandro2000 SuperSandro2000 marked this pull request as draft Jan 1, 2021
@spease spease mentioned this pull request Jan 20, 2021
9 tasks
@spease
Copy link
Contributor Author

@spease spease commented Jan 20, 2021

PR #110101 may supercede this. Both fixes in that are required for iOS to work (bootstrap?). The gccForLibs change has a similar effect as this one, it's just simpler.

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 21, 2021

Let's land #110251, and then revisit this. I think the way the conditions are written can be improved, but after I broke things on the last attempt, I want to do this in multiple steps.

@Ericson2314
Copy link
Member

@Ericson2314 Ericson2314 commented Jan 25, 2021

OK #110251 is merged. I agree with @SuperSandro2000 that the package shouldn't be null on the top level. But cc-wrapper should be smarter about "gcc for libgcc and friends" vs "gcc for libstdc++". If you want to take a stab at that, that would be much appreciated!

@stale
Copy link

@stale stale bot commented Jul 26, 2021

I marked this as stale due to inactivity. → More info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants