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

[haskell-updates] hackagePackages.neuron: Fix build #87206

Merged
merged 1 commit into from May 10, 2020

Conversation

maralorn
Copy link
Member

@maralorn maralorn commented May 7, 2020

WARNING: Do not merge, right now this uses a tarball from my personal server, not upstream, as a proof of concept.

Motivation for this change

I am trying to get the nice neuron app to build.

It is building, but I had to employ weird workarounds.

  1. The hackage package is not containing everything needed to actually compile, so pulling the source again from github. This is probably something we should fix on upstream in future releases.
  2. The program includes a nix-generate shell script at compiletime. I don‘t know how to do this nicely in nixpkgs without relying on IFD. Would be very open to suggestions.
  3. Right now the git version is not set during the build process. That is not a problem, but generated html outputs (UNKNOWN) were normally a git commit id would be.

I will contact upstream regarding all three points.

Things to be done
  • Add extra-sources on upstream
  • Change bash-script to not use IFD on upstream
  • New upstream release
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 execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@maralorn maralorn changed the title hackagePackages.neuron: Fix build [haskell-updates] hackagePackages.neuron: Fix build May 7, 2020
@maralorn maralorn mentioned this pull request May 7, 2020
@maralorn
Copy link
Member Author

maralorn commented May 7, 2020

@cdepillabout Since you where so helpful the other day. Do you have any idea what we can do about issue 2? I understand the urge of the package authors to include the nix store paths in there bash-script, but maintaining that script is configuration-common.nix is not a nice solution. Not sure what the best solution here is.
Kinda sad, that the authors embracing nix so much actually makes problem with including the package into nixpkgs.^^

@cdepillabout
Copy link
Member

The hackage package is not containing everything needed to actually compile, so pulling the source again from github.

In general, I think packages are considered "broken" if they don't contain all the files that are required to build when uploaded to Hackage.

The easiest thing to do here is to open an issue on the upstream repo (or even better, send a PR) that adds all required files to the extra-source-files section in the cabal file.

I think @srid is a big Nix user and Haskeller, so I imagine he will be sympathetic to fixing this.

After this is fixed, let's figure out what to do about 2 and 3. If @srid doesn't respond in a week or so, let's move forward with this PR and figure out how to fix it here in nixpkgs. In either case, please feel free to ping me here.

@maralorn
Copy link
Member Author

maralorn commented May 8, 2020

This PR is basically working now. Of course before we merge it we need a new neuron and rib release with my PR srid/neuron#164 merged.

Then I can remove all the manual overrides I have done here.

};
# neuron expects the neuron-search script to be in PATH at built-time.
executableHaskellDepends = drv.executableHaskellDepends or [ ] ++ [
(pkgs.runCommand "neuron-search" {
Copy link
Member Author

@maralorn maralorn May 8, 2020

Choose a reason for hiding this comment

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

@cdepillabout Do you think this makeWrapper solution is fine? Or is there a better way to do this?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is basically on the right track.

I'd propose a few small nitpicks:

  • Technically this shouldn't be executableHaskellDepends, but something like executableToolDepends or maybe executableSystemDepends (depending on whether it should go into nativeBuildInputs or just buildInputs).
  • There are helper functions to build PATHs. Grep for makeBinPath in the top-level lib/ directory in nixpkgs.
  • I'd pull out the whole runCommand part into a let-statement somewhere and give it a comment.

I guess it might be possible to do the whole wrapping step for neuron-search in like the postUnpack phase of the neuron build itself, instead of creating a separate derivation for it.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Okay makes sense. I think executableSystemDepends is the semantically the best.
  2. Yeah I wondered about that. Nice, will have a look.
  3. How far out? The runCommand needs the source. That‘s why it‘s in the derivation. If I don‘t find a solution for 4 I will just bump it as far as the scope permits.
  4. (Re: postUnpack) Yes, that would be much better. But I just couldn‘t get that to build. It‘s no problem to write a wrapper to put the script into $out/bin but neuron then needs to have that dir in the $PATH at compile time. Can you tell my how to do that?

Copy link
Member Author

Choose a reason for hiding this comment

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

TIL: Setting PATH in preBuild does not work, but it works in preConfigure or postUnpack.

Copy link
Member Author

Choose a reason for hiding this comment

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

I decided to use preConfigure instead of postUnpack, because there PWD is already the source and $out/bin is already created.
I think I have addressed all of this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I take back the $out/bin part. Had to create that myself.

@srid
Copy link
Contributor

srid commented May 8, 2020

rib 0.10 published to Hackage: https://hackage.haskell.org/package/rib-0.10.0.0

@maralorn
Copy link
Member Author

maralorn commented May 8, 2020

Okay, I think there is no big hurry here. So I‘ll wait until it‘s in hackage-packages.nix and rebase.

@srid
Copy link
Contributor

srid commented May 9, 2020

neuron 0.4 published to Hackage: https://hackage.haskell.org/package/neuron-0.4.0.0

@maralorn
Copy link
Member Author

maralorn commented May 9, 2020

Now just waiting for neuron 0.4 to hit haskell-packages.nix and then we are good to go.

@maralorn
Copy link
Member Author

Okay, thanks @srid so much for your very quick cooperation. From my perspective this is ready to be merged.

@maralorn maralorn marked this pull request as ready for review May 10, 2020 08:29
@cdepillabout
Copy link
Member

@maralorn This looks good to me! Thanks for working through this.

@cdepillabout cdepillabout merged commit 7b69f18 into NixOS:haskell-updates May 10, 2020
@maralorn maralorn deleted the fix-neuron branch May 10, 2020 11:03
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.

None yet

3 participants