Changes needed by Reflex Platform #23199

3noch · 2017-02-26T04:37:13Z

Motivation for this change

Trying to get Reflex-Platform to use latest nixpkgs. This is a start. I need to test this more too.

These changes are derived from NixOS/nixpkgs-channels@nixos-unstable...ryantrinkle:ghcjs-update

Some semantic differences between the original and my changes:

The original modified ghcjs/ghcjs.patch which was being used for all variations of GHCJS. Instead of changing that, I found that the changes only apply to ghcjsHEAD (which is using the ghc-8.0 branch) and not the default ghcjs package. So I made patches configurable and ghcjsHEAD uses a different patch file.
Here, if ghcLibdir was null, then ${libDir}/ghc_libdir_override would end up being written as an empty file. I changed this so that it does not run this code at all when ghcLibdir == null.

cc @ryantrinkle @ElvishJerricco

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.

Original: NixOS/nixpkgs-channels@17cea33

mention-bot · 2017-02-26T04:37:15Z

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

…GHCJS Original: NixOS/nixpkgs-channels@f311065

cstrahan

I have a couple questions about these changes:

cstrahan · 2017-02-26T06:03:20Z

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

@@ -194,7 +194,7 @@ stdenv.mkDerivation ({
    runHook preSetupCompilerEnvironment

    echo "Build with ${ghc}."
-    export PATH="${ghc}/bin:$PATH"
+    export PATH="${if ghc.isGhcjs or false then ghcEnv else ghc}/bin:$PATH"


What is the intent of this change? What is now on PATH that otherwise wouldn't?

The original commit reads "This helps support compiler plugins." @ryantrinkle Could you give a short description of why this is so (and why we would want ghcEnv for ghcjs but not native ghc)? I would also suggest putting the rationale as a comment somewhere near this line.

cstrahan · 2017-02-26T06:04:40Z

pkgs/development/compilers/ghcjs/base.nix

@@ -159,6 +163,7 @@ in mkDerivation (rec {
        --with-cabal ${cabal-install}/bin/cabal \
        --with-gmp-includes ${gmp.dev}/include \
        --with-gmp-libraries ${gmp.out}/lib
+      ${if isNull ghcLibdir then "" else ''echo "${ghcLibdir}" > "$out/lib/ghcjs-${version}/ghc_libdir"''}


What is ghc_libdir? A quick google search doesn't find anything insightful. Is this something being used specifically by Reflex/Obsidian in some way, or something more general?

The other commit doesn't provide any further details, I'm afraid (just says "wip").

@cstrahan, @ryantrinkle briefly explained to me that GHCJS has a "hack" that checks for the presence of this symlink and will use it as a way of adding GHCJS plugins. @ryantrinkle uses a plugin in reflex platform that is designed to add more performance optimizations. It is minimally effective at this point but he plans to expand it extensively.

However, for any sort of real accuracy or precision, we would need to hear directly from @ryantrinkle or @luite.

cstrahan · 2017-02-26T06:05:54Z

pkgs/development/haskell-modules/with-packages-wrapper.nix

@@ -46,14 +47,18 @@ let
  llvm          = lib.makeBinPath
                  ([ llvmPackages.llvm ]
                   ++ lib.optional stdenv.isDarwin llvmPackages.clang);
+  ghcLibdirLink = runCommand "ghc_libdir" {} ''
+    mkdir -p '${libDir}'
+    echo '${ghcLibdir}' > '${libDir}/ghc_libdir_override'


I assume this somehow related to ghc_libdir above; what does ghc_libdir_override do?

cc @ryantrinkle

greping the ghcjs source only shows ghc_libdir, so I suspect this was merely intended to plumb the path over in mv '${libDir}/ghc_libdir_override' '${libDir}/ghc_libdir'.

I agree this seems like a very roundabout way of accomplishing this.

Actually, I think I know why this is used. Last time I checked, buildEnv would try to create as few directories as possible, instead attempting to symlink as many existing directories as possible. If two entries in paths have the same directory, it would then be forced to create a new directory and symlink the contents of each of those two entries into that new directory.

Because there's an option (in this patch) to allow setting a default setting for ghc_libdir in the ghcjs/base.nix expression, we need to toss in something like ghcLibdirLink in paths to force baseEnv's hand, so the user can supply an overriding setting in ghcWithPackages. Gross, but it works.

pkgs.symlinkJoin behaves much better here, as it always creates directories in the derivation it produces, rather than trying to use symlinks everywhere.

@peti is there any downside you can see in switching from buildEnv to symlinkJoin here?

@cstrahan, I am not aware of any particular reason why we shouldn't be able to switch to symlinkJoin. I assume that the lndir tool on which that function is based is no less portable that our implementation of buildEnv is ... so IMHO we should use whichever one is more convenient. Generally speaking, being able to rely on bin and etc referring to real directories in a ghcWithPackages environment sounds like a good idea.

cstrahan · 2017-02-26T06:09:51Z

pkgs/development/haskell-modules/with-packages-wrapper.nix

@@ -102,6 +107,9 @@ buildEnv {
    done

    ${lib.optionalString hasLibraries "$out/bin/${ghcCommand}-pkg recache"}
+    ${lib.optionalString (isGhcjs && ghcLibdir != null) ''
+      mv '${libDir}/ghc_libdir_override' '${libDir}/ghc_libdir'


While I'm still not sure what we're trying to accomplish here: is there any reason not to inline the echo '${ghcLibdir}' > '${libDir}/ghc_libdir_override' here, instead of putting ghc_libdir_override in the symlink forest and then mving it over?

cstrahan · 2017-02-26T06:30:03Z

I thought that the ghcjs.patch on master applied cleanly for ghcjs HEAD, but if not, an updated diff is appreciated. And, of course, tweaks to work for iOS are great too.

I'm not sure what's going on with ghc_libdir and the use of ghcEnv (supposedly for compiler plugins support). If you can provide some justification for those changes, I can push this along -- or, alternatively, we might find a cleaner way to achieve whatever is desired here.

3noch · 2017-02-27T04:57:42Z

@cstrahan As it stands now, I would expect the patch phase to fail for ghcjsHEAD, but I have not confirmed that. However, by manually inspecting the line numbers, the new ghcjsHEAD.patch is definitely correct for ghc-8.0 branch, and it's not the same as master.

cstrahan · 2017-02-27T06:48:56Z

So, the patch as it stands actually applies successfully.

@3noch Here's how I'd like things to look: master...cstrahan:ghcjs-reflex

That's assuming @peti gives the 👍 on the symlinkJoin change. With that, I'll merge what I have.

3noch · 2017-02-27T14:34:29Z

@cstrahan Wow, you've gone above and beyond! So much better!

I'm confused how the patch applies, since the line numbers are certainly different. :/

cstrahan · 2017-02-27T21:34:50Z

@cstrahan Wow, you've gone above and beyond! So much better!

:)

I'm confused how the patch applies, since the line numbers are certainly different. :/

From the docs:

For context diffs, and to a lesser extent normal diffs, patch can detect when the line numbers mentioned in the patch are incorrect, and it attempts to find the correct place to apply each hunk of the patch. As a first guess, it takes the line number mentioned in the hunk, plus or minus any offset used in applying the previous hunk. If that is not the correct place, patch scans both forward and backward for a set of lines matching the context given in the hunk.

I'll give this one last sanity check tonight, check for anything I can simplify, and then merge to master.

3noch · 2017-02-27T23:19:39Z

@cstrahan TIL!

3noch · 2017-03-02T01:41:12Z

@cstrahan With @peti's approval above, can this be merged?

peti

👍

peti · 2017-03-02T07:45:19Z

pkgs/development/compilers/ghc/head.nix

@@ -105,7 +105,7 @@ in stdenv.mkDerivation (rec {
    "RANLIB=${stdenv.binutilsCross}/bin/${cross.config}-ranlib"
    "--target=${cross.config}"
    "--enable-bootstrap-with-devel-snapshot"
-  ];
+  ] ++ (lib.optional ("${cross.config}" == "aarch64-apple-darwin14") "--disable-large-address-space");


It's a minor detail, but I tend to prefer using stdenv.lib over expecting lib as an explicit argument. Makes no difference in practice, though, just my 2 cents.

Disable large address space in GHC iOS X compiler

39c6382

Original: NixOS/nixpkgs-channels@17cea33

Fix GHCJS HEAD patch; support GHCJS libdir link; use full ghcEnv for …

bf3cce9

…GHCJS Original: NixOS/nixpkgs-channels@f311065

3noch force-pushed the reflex-platform-changes branch from b69e2e4 to bf3cce9 Compare February 26, 2017 04:41

cstrahan reviewed Feb 26, 2017

View reviewed changes

peti approved these changes Mar 2, 2017

View reviewed changes

cstrahan closed this in 4b77d42 Mar 2, 2017

3noch mentioned this pull request Mar 2, 2017

Pin nixpkgs to an unmodified upstream LTS revision reflex-frp/reflex-platform#24

Closed

3 tasks

3noch deleted the reflex-platform-changes branch March 5, 2017 00:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes needed by Reflex Platform #23199

Changes needed by Reflex Platform #23199

3noch commented Feb 26, 2017 •

edited

mention-bot commented Feb 26, 2017

cstrahan left a comment

cstrahan Feb 26, 2017

cstrahan Feb 26, 2017

cstrahan Feb 26, 2017

cstrahan Feb 26, 2017

3noch Feb 27, 2017

cstrahan Feb 26, 2017

3noch Feb 27, 2017

cstrahan Feb 27, 2017

3noch Feb 27, 2017

cstrahan Feb 27, 2017 •

edited

peti Feb 27, 2017

cstrahan Feb 26, 2017

cstrahan commented Feb 26, 2017 •

edited

3noch commented Feb 27, 2017

cstrahan commented Feb 27, 2017 •

edited

3noch commented Feb 27, 2017

cstrahan commented Feb 27, 2017

3noch commented Feb 27, 2017

3noch commented Mar 2, 2017 •

edited

peti left a comment

peti Mar 2, 2017

Changes needed by Reflex Platform #23199

Changes needed by Reflex Platform #23199

Conversation

3noch commented Feb 26, 2017 • edited

Motivation for this change

Things done

mention-bot commented Feb 26, 2017

cstrahan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstrahan Feb 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cstrahan commented Feb 26, 2017 • edited

3noch commented Feb 27, 2017

cstrahan commented Feb 27, 2017 • edited

3noch commented Feb 27, 2017

cstrahan commented Feb 27, 2017

3noch commented Feb 27, 2017

3noch commented Mar 2, 2017 • edited

peti left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3noch commented Feb 26, 2017 •

edited

cstrahan Feb 27, 2017 •

edited

cstrahan commented Feb 26, 2017 •

edited

cstrahan commented Feb 27, 2017 •

edited

3noch commented Mar 2, 2017 •

edited