haskell: use GHCJS to build Setup.hs for GHCJS packages #23614

ljli · 2017-03-07T20:32:57Z

One of the consequences of how GHCJS is compiled (first the stage1 packages, which do not include Cabal, then Cabal, and then all the stage2 packages with the previously compiled Cabal) is that the GHCJS native package-db does not include Cabal (unlike GHC). It follows that Cabal packages we want to compile with GHCJS need to depend explicitly on Cabal otherwise there will be no Cabal in the package-db to compile the Setup.hs. This wasn't a problem before since we used GHC to compile the Setup.hs which houses a Cabal in it's global package-db. I accounted for this by declaring Cabal as a Setup dependency for all sensible GHCJS packages. I guess that makes sense semantically, nonetheless I thought about different solutions, but I did not come up with something better.

With GHCJS and Cabal, executables from cabal packages are compiled to a bunch of .js files housed in folders like <exe-name>.jsexe. I provide nodejs wrappers automatically as <exe-name>, which seems to work fine. Tested with HSColour.

For testing #23611 should be applied, too, otherwise the Setup.hs compilations are of not much use and the tasty-ant-xml caveat mentioned there applies here as well.

The optionalString to optional refactoring is done, because otherwise each conditional flag that is added triggers a rebuild of everything (even if the flag is not activated) that depends on the builder, just because a whitespace is added into the build scripts when the flags are concatenated.

Motivation for this change

Currently we build the Setup.hs of cabal packages with GHC even if we use GHCJS for the actual compilation. This seems to work in general, but makes GHC as well as GHCJS a build time dependency for every cabal package compiled with GHCJS. There is also a bug in that we pass in generic-builder.nix a GHC or GHCJS package db to a command that is always GHC. GHC can not use a package-db from GHCJS, this becomes a problem if the cabal package has a dependency on a package that is also required for compiling Setup.hs, in that case the GHC native package gets shadowed.

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
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.

mention-bot · 2017-03-07T20:32:59Z

@ljli, thanks for your PR! By analyzing the history of the files in this pull request, we identified @peti, @cstrahan and @edolstra to be potential reviewers.

peti

Please undo the unrelated changes that introduce flatten and clarify (or remove) the added bash argument to the generic builder.

peti · 2017-03-08T08:27:04Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -106,22 +106,22 @@ let
  crossCabalFlagsString =
    stdenv.lib.optionalString isCross (" " + stdenv.lib.concatStringsSep " " crossCabalFlags);

-  defaultConfigureFlags = [
+  defaultConfigureFlags = flatten [


I don't like this change. Having a list of optional strings works just fine, IMHO, and I don't see what we gain by turning that into a list of lists that will be flattened. That feels like an unnecessary complication, and the change is certainly off-topic for this PR, which says it's about improving GHCJS support.

It is a refactoring unrelated to the change, so I'm going to take it out.

Just for elaboration: The motivation (as mentioned in the PR) is that if one adds a flag to defaultConfigureFlags that is conditional on isGhcjs with optionalString, a reasonable expectation, IMHO, would be that only GHCJS package derivations change and have to be rebuilt. This is not the case, since the flags are joined by concatStringsSep " ", every empty string contributes a additional space char. Oversensitive builder scripts are a problem in general, but here it seemed especially desirable to be able to change for example the default configuration for GHCJS packages without having to rebuild the whole GHC package sets, too, for really no good reason.

Now that I think again about it, it would probably be less invasive and more robust, to use a different function than concatStringsSep to achieve the same effect.

peti · 2017-03-08T08:27:28Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -131,10 +131,11 @@ let
    "--configure-option=--host=${ghc.cross.config}"
  ] ++ crossCabalFlags);

-  setupCompileFlags = [


peti · 2017-03-08T08:29:27Z

pkgs/development/haskell-modules/generic-builder.nix

@@ -1,5 +1,5 @@
 { stdenv, fetchurl, ghc, pkgconfig, glibcLocales, coreutils, gnugrep, gnused
-, jailbreak-cabal, hscolour, cpphs, nodePackages
+, jailbreak-cabal, hscolour, cpphs, bash, nodejs


The added bash argument here doesn't seem to be used anywhere?

Indeed, this was from a previous iteration, where I used bash in the .jsexe wrappers.

peti · 2017-03-14T15:09:02Z

I would like to merge this change, but there are conflicts in the generic builder. Can you please force-push a version that's rebased relative to the current master branch?

ljli · 2017-03-24T22:12:26Z

Should be good to merge now.

peti · 2017-03-24T22:21:58Z

pkgs/development/haskell-modules/configuration-ghcjs.nix

 let
  removeLibraryHaskellDepends = pnames: depends:
    builtins.filter (e: !(builtins.elem (e.pname or "") pnames)) depends;
+  addSetupDependGlobally = excludes: pset: dep:


Wouldn't it be easier to wrap the mkDerivation function in the package set to add those dependencies? That would be one override, rather than one override per package in the whole set.

ljli · 2017-03-25T04:27:14Z

Is this better?

peti · 2017-03-27T12:12:09Z

Thank you very much for the effort you've put into this PR! I've merged it to haskell-updates in 7387bd6bc2e863f134746f0a272fa9b0063dc006 to get some test builds on Hydra, and then it should arrive on master within the next day or two.

Closes NixOS#23614.

shlevy · 2017-04-06T00:57:52Z

This breaks hyperlinking Cabal, likely slows down builds (have to fire up a node instance to run Setup!), and, most seriously, breaks builds using hsc2hs (https://github.com/ghcjs/ghcjs/blob/8c30beb939dadcb949b922856735080699d1d986/src-bin/Hsc2Hs.hs#L23-L27). We could fix the latter by using ghcjs's hcs2hs wrapper, but it's not clear to me why this change was desirable in the first place and I'd much rather go back to using ghc to compile `Setup. @ljli, @peti, thoughts? Why was this change valuable?

ljli · 2017-04-06T05:08:22Z

As written initially the goal was to reduce the build dependencies for GHCJS packages from two compilers (GHCJS and GHC) to one (GHCJS). To me it seems odd that we would want to bring in a second independent compiler (weighing in at 1GB) for such a mundane task as compiling the Setup.hs.

Setting hyperlinkSource = false for cabal was just to break a bootstrap cycle involving hscolour. We should be able to rebuild it at a later stage with hyperlinked haddock. I will look into it.

Your PR regarding hsc2hs looks good to me. I don't know what exactly the problem is (how to use C bindings with GHCJS isn't obvious to me in the first place), but using hsc2hs from GHCJS sounds like the right default for GHCJS packages.

The overhead of nodejs-run GHCJS-compiled JavaScript vs. GHC-compiled native code is a conceivable argument to bring in GHC as an optimization. I would be surprised if the difference were not dwarfed by the actual compilation done by GHCJS, after all Setup.hs typically does little more than call out to binaries, so I would probably try to benchmark it before opting against the conceptual simpler default. But if you feel strongly about it, by all means revert the change. A clean cross-compiler architecture should be able to support that. We need to distinguish between a compiler A (host -> host) used to compile the Setup.hs and a compiler B (host -> target) which is driven by the A compiled Setup.hs. We already have some logic for that in haskell-modules/generic-builder.nix, but e.g. miss a separate package-db for setup dependencies which need to come from the default pkg set for A not from B. That the setupHaskellDepends dependencies are resolved with the rest of the actual package dependencies via callPackage in the same package set is actually only sound when A = B, so that would have to be changed as well (and actually should be for the benefit of general cross-compilation independent of this issue)

shlevy · 2017-04-06T07:12:06Z

That makes sense, thanks. Unfortunately, the ghcjs hsc2hs uses the ghc hsc2hs under the hood, so we can't lose the dependency after all ☹️ Any way, my biggest concern is just getting things building again, so I'll leave it to you and @peti to decide which approach to use.

This is required when using ghcjs to compile Setup.hs, which we do since #23614. See comments on ghcjs/ghcjs@c35350a

peti · 2017-04-06T07:32:57Z

/me knows nothing about ghcjs. I'll be happy with whatever solution those people come up who actually use it.

domenkozar added the 6.topic: haskell label Mar 7, 2017

ljli force-pushed the hs-ghcjs-native-setup branch from a286bd1 to 3ffd28f Compare March 7, 2017 22:24

peti requested changes Mar 8, 2017

View reviewed changes

ljli force-pushed the hs-ghcjs-native-setup branch 2 times, most recently from 98f57ef to a958f37 Compare March 8, 2017 12:50

ljli mentioned this pull request Mar 8, 2017

haskell: ghcjs packages: fix ghcjs-dom-jsffi with newer Cabal #23615

Merged

7 tasks

peti approved these changes Mar 14, 2017

View reviewed changes

ljli force-pushed the hs-ghcjs-native-setup branch from a958f37 to 02c6859 Compare March 14, 2017 15:17

peti reviewed Mar 24, 2017

View reviewed changes

haskell: use GHCJS to build Setup.hs for GHCJS packages

b22690d

ljli force-pushed the hs-ghcjs-native-setup branch from 02c6859 to b22690d Compare March 25, 2017 04:26

peti closed this in 2553ceb Mar 29, 2017

Krofek pushed a commit to Krofek/nixpkgs that referenced this pull request Mar 30, 2017

haskell: use GHCJS to build Setup.hs for GHCJS packages

c0c8469

Closes NixOS#23614.

ljli mentioned this pull request Apr 3, 2017

ghcjs (head): Fix infinite recursion ... #24510

Merged

7 tasks

shlevy mentioned this pull request Apr 6, 2017

haskell generic builder: Use ghcjs's hsc2hs. #24663

Merged

shlevy added a commit that referenced this pull request Apr 6, 2017

haskell generic builder: Use ghcjs's hsc2hs.

5e98436

This is required when using ghcjs to compile Setup.hs, which we do since #23614. See comments on ghcjs/ghcjs@c35350a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

haskell: use GHCJS to build Setup.hs for GHCJS packages #23614

haskell: use GHCJS to build Setup.hs for GHCJS packages #23614

ljli commented Mar 7, 2017 •

edited

mention-bot commented Mar 7, 2017

peti left a comment

peti Mar 8, 2017

ljli Mar 8, 2017

peti Mar 8, 2017

peti Mar 8, 2017

ljli Mar 8, 2017

peti commented Mar 14, 2017

ljli commented Mar 24, 2017

peti Mar 24, 2017

ljli commented Mar 25, 2017

peti commented Mar 27, 2017

shlevy commented Apr 6, 2017

ljli commented Apr 6, 2017

shlevy commented Apr 6, 2017

peti commented Apr 6, 2017

haskell: use GHCJS to build Setup.hs for GHCJS packages #23614

haskell: use GHCJS to build Setup.hs for GHCJS packages #23614

Conversation

ljli commented Mar 7, 2017 • edited

Motivation for this change

Things done

mention-bot commented Mar 7, 2017

peti left a comment

Choose a reason for hiding this comment

peti Mar 8, 2017

Choose a reason for hiding this comment

ljli Mar 8, 2017

Choose a reason for hiding this comment

peti Mar 8, 2017

Choose a reason for hiding this comment

peti Mar 8, 2017

Choose a reason for hiding this comment

ljli Mar 8, 2017

Choose a reason for hiding this comment

peti commented Mar 14, 2017

ljli commented Mar 24, 2017

peti Mar 24, 2017

Choose a reason for hiding this comment

ljli commented Mar 25, 2017

peti commented Mar 27, 2017

shlevy commented Apr 6, 2017

ljli commented Apr 6, 2017

shlevy commented Apr 6, 2017

peti commented Apr 6, 2017

ljli commented Mar 7, 2017 •

edited