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.

Sign up for GitHub

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

Jump to bottom

Haskell builder: Use $abi/$libname as --libsubdir #42224

Closed
wants to merge 1 commit into from
Closed

Haskell builder: Use $abi/$libname as --libsubdir #42224

wants to merge 1 commit into from

Conversation

alexbiehl
Copy link
Contributor

@alexbiehl alexbiehl commented Jun 19, 2018 •
edited

Motivation for this change

This is an attempt on #40128. I should definitely take a look at the second point of Edwards answer: -rpath for linux. Maybe some could help me there. --ghc-option doesn't support path templates like $abi or $libname.

/cc @shlevy @grahamc

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@alexbiehl alexbiehl requested a review from peti as a code owner June 19, 2018 11:22
@peti
Copy link
Member

peti commented Jun 22, 2018

I don't understand what this change does, exactly, and why it apparently achieves better backpack support.

@alexbiehl
Copy link
Contributor Author

@peti Please see @ezyang comment on the matter #40128 (comment).

@alexbiehl
Copy link
Contributor Author

@peti does it make sense? $pkgid does not identify a package uniquely (basically it is pure coincidence that it worked before). Especially one that uses backpack. The combination of $abi and $libname however does.

@ezyang or @hvr might give a more thorough answer.

@ezyang
Copy link

ezyang commented Jun 24, 2018

@alexbiehl Does the change work? :) Naively I'd expect this could cause more thrashing in Nix, because the default generated values for these are not very stable.

@alexbiehl
Copy link
Contributor Author

alexbiehl commented Jun 25, 2018 •
edited

@ezyang Yes it works! It will definitely trash the haskell libs in your nix store the first time you use it. But I haven't experience any others yet.

@shlevy
Copy link
Member

shlevy commented Jun 25, 2018

@ezyang Can you expand on the thrashing a bit? Is this likely to be a source of impurities between otherwise identical nix builds on different machines or something?

@shlevy
Copy link
Member

shlevy commented Jun 25, 2018

@peti Can you review @ezyang's assessment and let us know if this is a decent path forward or not?

peti
peti previously requested changes Jul 19, 2018
View reviewed changes
Copy link
Member

@peti peti left a comment

Choose a reason for hiding this comment

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

I am basically in favor of this change. Before we merge, however, I would like to see a clear indication that the resulting packages work, particularly when used with ghcWithPackages and ghcWithHoogle because this path change has the potential to break assumptions in other code.

In other words, this change needs testing before it can go forward.

@shlevy
Copy link
Member

shlevy commented Jul 19, 2018

@alexbiehl We've been actively using this for a while, right?

@alexbiehl
Copy link
Contributor Author

alexbiehl commented Jul 19, 2018 •
edited

@shlevy correct. We didn't find any error, yet. But I don't know really know which parts of nixpkgs are stressed in our nix infrastructure. Specifically, I don't know if we use something like ghcWithPackages or ghcWithHoogle.

@shlevy
Copy link
Member

shlevy commented Jul 19, 2018

We rely on ghcWithPackages for our nix-shells

@peti peti dismissed their stale review July 20, 2018 07:19

Well, if you are confident that these changes work properly, then please feel free to merge.

@shlevy
Copy link
Member

shlevy commented Sep 3, 2018

Merged into haskell-updates

@shlevy shlevy closed this Sep 3, 2018
@alexbiehl alexbiehl deleted the fix-ghc-backpack branch September 3, 2018 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peti peti peti left review comments

Assignees
No one assigned
Labels
6.topic: haskell 10.rebuild-darwin: 501+ 10.rebuild-linux: 501+ 12. first-time contribution
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@alexbiehl @peti @ezyang @shlevy @GrahamcOfBorg @Janik-Haag