shellFor: Refactor for consistency and cross #76349
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Ericson2314
requested review from
basvandijk and
cdepillabout
as code owners
Could you add more documentation in the code about this? There is a huge lack of documentation about cross-compilation. When I'm making changes to Haskell stuff, I have very little confidence I'm not breaking cross-complation-related things. Also, do you have any suggestion on how I can test cross-compilation stuff? Should I buy a rasberry-pi raspberry pi and cross compile Haskell stuff for it? One more thing: would it be possible to write a test for this new |
|
@Ericson2314 I left a lot of comments above, but I should also add that I am very grateful you guys are upstreaming stuff from obsidian :-) |
Hehe thank you, they are all good questions I will be sure we get to all of them and document everything thoroughly before anything is merged. Upstreaming most of reflex-platform is something I've been waiting years to finally have the time to do, so I'm as excited as you are. :) |
|
We will add some tests in |
pkgs/test/default.nix
Outdated
| @@ -22,6 +22,8 @@ with pkgs; | |||
| cc-wrapper-libcxx-7 = callPackage ./cc-wrapper { stdenv = llvmPackages_7.libcxxStdenv; }; | |||
| stdenv-inputs = callPackage ./stdenv-inputs { }; | |||
|
|
|||
| haskell = callPackage ./haskell { }; | |||
There was a problem hiding this comment.
For this to be tested, we also need an entry in pkgs/top-level/release.nix.
| pkgconfigDepends | ||
| setupHaskellDepends | ||
| ; | ||
| } // stdenv.lib.optionalAttrs doCheck { |
There was a problem hiding this comment.
I don't think this needs to be optional given we are calculating regardless of whether doCheck / doBenchmark is on.
Ericson2314
force-pushed
the
work-on-multi-shellFor
branch
from
92cdb49
to
df0e79c
Compare
ofborg
bot
added
6.topic: stdenv
Ericson2314
force-pushed
the
work-on-multi-shellFor
branch
from
df0e79c
to
15ebdbe
Compare
| @@ -1,4 +1,4 @@ | |||
| { lib, stdenv, ghc, llvmPackages, packages, symlinkJoin, makeWrapper | |||
| { lib, stdenv, ghc, llvmPackages, packages, buildEnv, makeWrapper | |||
There was a problem hiding this comment.
This one was reverted. I think this might just be a github diff bug, but we should verify this change isn’t getting into master accidentally.
#77523 corrects it in the 19.03/19.09/master merge-base.
matthewbauer
changed the title
|
Real diff is: diff --git a/pkgs/development/haskell-modules/generic-builder.nix b/pkgs/development/haskell-modules/generic-builder.nix
index 5410fccf0bb..4c552d64d9a 100644
--- a/pkgs/development/haskell-modules/generic-builder.nix
+++ b/pkgs/development/haskell-modules/generic-builder.nix
@@ -1,5 +1,6 @@
{ stdenv, buildPackages, buildHaskellPackages, ghc
-, jailbreak-cabal, hscolour, cpphs, nodejs, shellFor
+, jailbreak-cabal, hscolour, cpphs, nodejs
+, ghcWithHoogle, ghcWithPackages
}:
let
@@ -206,21 +207,28 @@ let
optionals doCheck testPkgconfigDepends ++ optionals doBenchmark benchmarkPkgconfigDepends;
depsBuildBuild = [ nativeGhc ];
- nativeBuildInputs = [ ghc removeReferencesTo ] ++ optional (allPkgconfigDepends != []) pkgconfig ++
- setupHaskellDepends ++
- buildTools ++ libraryToolDepends ++ executableToolDepends ++
- optionals doCheck testToolDepends ++
- optionals doBenchmark benchmarkToolDepends;
+ collectedToolDepends =
+ buildTools ++ libraryToolDepends ++ executableToolDepends ++
+ optionals doCheck testToolDepends ++
+ optionals doBenchmark benchmarkToolDepends;
+ nativeBuildInputs =
+ [ ghc removeReferencesTo ] ++ optional (allPkgconfigDepends != []) pkgconfig ++
+ setupHaskellDepends ++ collectedToolDepends;
propagatedBuildInputs = buildDepends ++ libraryHaskellDepends ++ executableHaskellDepends ++ libraryFrameworkDepends;
- otherBuildInputs = extraLibraries ++ librarySystemDepends ++ executableSystemDepends ++ executableFrameworkDepends ++
- allPkgconfigDepends ++
- optionals doCheck (testDepends ++ testHaskellDepends ++ testSystemDepends ++ testFrameworkDepends) ++
- optionals doBenchmark (benchmarkDepends ++ benchmarkHaskellDepends ++ benchmarkSystemDepends ++ benchmarkFrameworkDepends);
-
-
- allBuildInputs = propagatedBuildInputs ++ otherBuildInputs ++ depsBuildBuild ++ nativeBuildInputs;
- isHaskellPartition =
- stdenv.lib.partition isHaskellPkg allBuildInputs;
+ otherBuildInputsHaskell =
+ optionals doCheck (testDepends ++ testHaskellDepends) ++
+ optionals doBenchmark (benchmarkDepends ++ benchmarkHaskellDepends);
+ otherBuildInputsSystem =
+ extraLibraries ++ librarySystemDepends ++ executableSystemDepends ++ executableFrameworkDepends ++
+ allPkgconfigDepends ++
+ optionals doCheck (testSystemDepends ++ testFrameworkDepends) ++
+ optionals doBenchmark (benchmarkSystemDepends ++ benchmarkFrameworkDepends);
+ # TODO next rebuild just define as `otherBuildInputsHaskell ++ otherBuildInputsSystem`
+ otherBuildInputs =
+ extraLibraries ++ librarySystemDepends ++ executableSystemDepends ++ executableFrameworkDepends ++
+ allPkgconfigDepends ++
+ optionals doCheck (testDepends ++ testHaskellDepends ++ testSystemDepends ++ testFrameworkDepends) ++
+ optionals doBenchmark (benchmarkDepends ++ benchmarkHaskellDepends ++ benchmarkSystemDepends ++ benchmarkFrameworkDepends);
setupCommand = "./Setup";
@@ -462,17 +470,61 @@ stdenv.mkDerivation ({
runHook postInstall
'';
- passthru = passthru // {
+ passthru = passthru // rec {
inherit pname version;
compiler = ghc;
+ # All this information is intended just for `shellFor`. It should be
+ # considered unstable and indeed we knew how to keep it private we would.
+ getCabalDeps = {
+ inherit
+ buildDepends
+ buildTools
+ executableFrameworkDepends
+ executableHaskellDepends
+ executablePkgconfigDepends
+ executableSystemDepends
+ executableToolDepends
+ extraLibraries
+ libraryFrameworkDepends
+ libraryHaskellDepends
+ libraryPkgconfigDepends
+ librarySystemDepends
+ libraryToolDepends
+ pkgconfigDepends
+ setupHaskellDepends
+ ;
+ } // stdenv.lib.optionalAttrs doCheck {
+ inherit
+ testDepends
+ testFrameworkDepends
+ testHaskellDepends
+ testPkgconfigDepends
+ testSystemDepends
+ testToolDepends
+ ;
+ } // stdenv.lib.optionalAttrs doBenchmark {
+ inherit
+ benchmarkDepends
+ benchmarkFrameworkDepends
+ benchmarkHaskellDepends
+ benchmarkPkgconfigDepends
+ benchmarkSystemDepends
+ benchmarkToolDepends
+ ;
+ };
- getBuildInputs = {
+ # Attributes for the old definition of `shellFor`. Should be removed but
+ # this predates the warning at the top of `getCabalDeps`.
+ getBuildInputs = rec {
inherit propagatedBuildInputs otherBuildInputs allPkgconfigDepends;
haskellBuildInputs = isHaskellPartition.right;
systemBuildInputs = isHaskellPartition.wrong;
+ isHaskellPartition = stdenv.lib.partition
+ isHaskellPkg
+ (propagatedBuildInputs ++ otherBuildInputs ++ depsBuildBuild ++ nativeBuildInputs);
};
isHaskellLibrary = isLibrary;
@@ -485,10 +537,64 @@ stdenv.mkDerivation ({
# TODO: fetch the self from the fixpoint instead
haddockDir = self: if doHaddock then "${docdir self.doc}/html" else null;
- env = shellFor {
- packages = p: [ drv ];
- inherit shellHook;
- };
+ # Creates a derivation containing all of the necessary dependencies for building the
+ # parent derivation. The attribute set that it takes as input can be viewed as:
+ #
+ # { withHoogle }
+ #
+ # The derivation that it builds contains no outpaths because it is meant for use
+ # as an environment
+ #
+ # # Example use
+ # # Creates a shell with all of the dependencies required to build the "hello" package,
+ # # and with python:
+ #
+ # > nix-shell -E 'with (import <nixpkgs> {}); \
+ # > haskell.packages.ghc865.hello.envFunc { buildInputs = [ python ]; }'
+ envFunc = { withHoogle ? false }:
+ let
+ name = "ghc-shell-for-${drv.name}";
+
+ withPackages = if withHoogle then ghcWithHoogle else ghcWithPackages;
+
+ # We use the `ghcWithPackages` function from `buildHaskellPackages` if we
+ # want a shell for the sake of cross compiling a package. In the native case
+ # we don't use this at all, and instead put the setupDepends in the main
+ # `ghcWithPackages`. This way we don't have two wrapper scripts called `ghc`
+ # shadowing each other on the PATH.
+ ghcEnvForBuild =
+ assert isCross;
+ buildHaskellPackages.ghcWithPackages (_: setupHaskellDepends);
+
+ ghcEnv = withPackages (_:
+ otherBuildInputsHaskell ++
+ propagatedBuildInputs ++
+ stdenv.lib.optionals (!isCross) setupHaskellDepends);
+
+ ghcCommandCaps = stdenv.lib.toUpper ghcCommand';
+ in stdenv.mkDerivation ({
+ inherit name shellHook;
+
+ depsBuildBuild = stdenv.lib.optional isCross ghcEnvForBuild;
+ nativeBuildInputs =
+ [ ghcEnv ] ++ optional (allPkgconfigDepends != []) pkgconfig ++
+ collectedToolDepends;
+ buildInputs =
+ otherBuildInputsSystem;
+ phases = ["installPhase"];
+ installPhase = "echo $nativeBuildInputs $buildInputs > $out";
+ LANG = "en_US.UTF-8";
+ LOCALE_ARCHIVE = stdenv.lib.optionalString (stdenv.hostPlatform.libc == "glibc") "${buildPackages.glibcLocales}/lib/locale/locale-archive";
+ "NIX_${ghcCommandCaps}" = "${ghcEnv}/bin/${ghcCommand}";
+ "NIX_${ghcCommandCaps}PKG" = "${ghcEnv}/bin/${ghcCommand}-pkg";
+ # TODO: is this still valid?
+ "NIX_${ghcCommandCaps}_DOCDIR" = "${ghcEnv}/share/doc/ghc/html";
+ "NIX_${ghcCommandCaps}_LIBDIR" = if ghc.isHaLVM or false
+ then "${ghcEnv}/lib/HaLVM-${ghc.version}"
+ else "${ghcEnv}/lib/${ghcCommand}-${ghc.version}";
+ });
+
+ env = envFunc { };
};
diff --git a/pkgs/development/haskell-modules/make-package-set.nix b/pkgs/development/haskell-modules/make-package-set.nix
index e2d01c5798f..9ba25e09db9 100644
--- a/pkgs/development/haskell-modules/make-package-set.nix
+++ b/pkgs/development/haskell-modules/make-package-set.nix
@@ -38,12 +38,12 @@ let
inherit (stdenv) buildPlatform hostPlatform;
inherit (stdenv.lib) fix' extends makeOverridable;
- inherit (haskellLib) overrideCabal getBuildInputs;
+ inherit (haskellLib) overrideCabal;
mkDerivationImpl = pkgs.callPackage ./generic-builder.nix {
inherit stdenv;
nodejs = buildPackages.nodejs-slim;
- inherit (self) buildHaskellPackages ghc shellFor;
+ inherit (self) buildHaskellPackages ghc ghcWithHoogle ghcWithPackages;
inherit (self.buildHaskellPackages) jailbreak-cabal;
hscolour = overrideCabal self.buildHaskellPackages.hscolour (drv: {
isLibrary = false;
@@ -258,6 +258,8 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
# packages themselves. Using nix-shell on this derivation will
# give you an environment suitable for developing the listed
# packages with an incremental tool like cabal-install.
+ # In addition to the "packages" arg and "withHoogle" arg, anything that
+ # can be passed into stdenv.mkDerivation can be included in the input attrset
#
# # default.nix
# with import <nixpkgs> {};
@@ -268,9 +270,11 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
# })
#
# # shell.nix
+ # let pkgs = import <nixpkgs> {} in
# (import ./.).shellFor {
# packages = p: [p.frontend p.backend p.common];
# withHoogle = true;
+ # buildInputs = [ pkgs.python ];
# }
#
# -- cabal.project
@@ -280,49 +284,41 @@ in package-set { inherit pkgs stdenv callPackage; } self // {
# common/
#
# bash$ nix-shell --run "cabal new-build all"
+ # bash$ nix-shell --run "python"
shellFor = { packages, withHoogle ? false, ... } @ args:
let
- selected = packages self;
-
- packageInputs = map getBuildInputs selected;
-
- name = if pkgs.lib.length selected == 1
- then "ghc-shell-for-${(pkgs.lib.head selected).name}"
- else "ghc-shell-for-packages";
-
- # If `packages = [ a b ]` and `a` depends on `b`, don't build `b`,
- # because cabal will end up ignoring that built version, assuming
- # new-style commands.
- haskellInputs = pkgs.lib.filter
- (input: pkgs.lib.all (p: input.outPath != p.outPath) selected)
- (pkgs.lib.concatMap (p: p.haskellBuildInputs) packageInputs);
- systemInputs = pkgs.lib.concatMap (p: p.systemBuildInputs) packageInputs;
-
- withPackages = if withHoogle then self.ghcWithHoogle else self.ghcWithPackages;
- ghcEnv = withPackages (p: haskellInputs);
- nativeBuildInputs = pkgs.lib.concatMap (p: p.nativeBuildInputs) selected;
-
- ghcCommand' = if ghc.isGhcjs or false then "ghcjs" else "ghc";
- ghcCommand = "${ghc.targetPrefix}${ghcCommand'}";
- ghcCommandCaps= pkgs.lib.toUpper ghcCommand';
-
- mkDrvArgs = builtins.removeAttrs args ["packages" "withHoogle"];
- in pkgs.stdenv.mkDerivation (mkDrvArgs // {
- name = mkDrvArgs.name or name;
-
- buildInputs = systemInputs ++ mkDrvArgs.buildInputs or [];
- nativeBuildInputs = [ ghcEnv ] ++ nativeBuildInputs ++ mkDrvArgs.nativeBuildInputs or [];
- phases = ["installPhase"];
- installPhase = "echo $nativeBuildInputs $buildInputs > $out";
- LANG = "en_US.UTF-8";
- LOCALE_ARCHIVE = pkgs.lib.optionalString (stdenv.hostPlatform.libc == "glibc") "${buildPackages.glibcLocales}/lib/locale/locale-archive";
- "NIX_${ghcCommandCaps}" = "${ghcEnv}/bin/${ghcCommand}";
- "NIX_${ghcCommandCaps}PKG" = "${ghcEnv}/bin/${ghcCommand}-pkg";
- # TODO: is this still valid?
- "NIX_${ghcCommandCaps}_DOCDIR" = "${ghcEnv}/share/doc/ghc/html";
- "NIX_${ghcCommandCaps}_LIBDIR" = if ghc.isHaLVM or false
- then "${ghcEnv}/lib/HaLVM-${ghc.version}"
- else "${ghcEnv}/lib/${ghcCommand}-${ghc.version}";
+ combinedPackageFor = packages:
+ let
+ selected = packages self;
+
+ pname = if pkgs.lib.length selected == 1
+ then (pkgs.lib.head selected).name
+ else "packages";
+
+ # If `packages = [ a b ]` and `a` depends on `b`, don't build `b`,
+ # because cabal will end up ignoring that built version, assuming
+ # new-style commands.
+ combinedPackages = pkgs.lib.filter
+ (input: pkgs.lib.all (p: input.outPath or null != p.outPath) selected);
+
+ # Returns an attrset containing a combined list packages' inputs for each
+ # stage of the build process
+ packageInputs = pkgs.lib.zipAttrsWith
+ (_: pkgs.lib.concatMap combinedPackages)
+ (map (p: p.getCabalDeps) selected);
+
+ genericBuilderArgs = {
+ inherit pname;
+ version = "0";
+ license = null;
+ } // packageInputs;
+
+ in self.mkDerivation genericBuilderArgs;
+
+ envFuncArgs = builtins.removeAttrs args [ "packages" ];
+ in (combinedPackageFor packages).env.overrideAttrs (old: envFuncArgs // {
+ nativeBuildInputs = old.nativeBuildInputs ++ envFuncArgs.nativeBuildInputs or [];
+ buildInputs = old.buildInputs ++ envFuncArgs.buildInputs or [];
});
ghc = ghc // {
diff --git a/pkgs/test/default.nix b/pkgs/test/default.nix
index eb0711b8885..f62d208d22d 100644
--- a/pkgs/test/default.nix
+++ b/pkgs/test/default.nix
@@ -26,6 +26,8 @@ with pkgs;
cc-wrapper-libcxx-9 = callPackage ./cc-wrapper { stdenv = llvmPackages_9.libcxxStdenv; };
stdenv-inputs = callPackage ./stdenv-inputs { };
+ haskell-shellFor = callPackage ./haskell-shellFor { };
+
cc-multilib-gcc = callPackage ./cc-wrapper/multilib.nix { stdenv = gccMultiStdenv; };
cc-multilib-clang = callPackage ./cc-wrapper/multilib.nix { stdenv = clangMultiStdenv; };
|
Ericson2314
force-pushed
the
work-on-multi-shellFor
branch
from
15ebdbe
to
d8e8eba
Compare
| @@ -22,6 +22,8 @@ with pkgs; | |||
| cc-wrapper-libcxx-7 = callPackage ./cc-wrapper { stdenv = llvmPackages_7.libcxxStdenv; }; | |||
| stdenv-inputs = callPackage ./stdenv-inputs { }; | |||
|
|
|||
| haskell-shellFor = callPackage ./haskell-shellFor { }; | |||
There was a problem hiding this comment.
It looks like this file doesn't exist yet in this PR? Was this an oversight? Or is it still being written?
There was a problem hiding this comment.
@jmininger did we forget to commit??
This makes it work like work-on-multi from Reflex Platform. In particular, rather than making `.env` from `shellFor`, we make `.env` the primitive, and `shellFor` works by combining together the arguments of all the packages to `generic-builder` and taking the `.env` of the resulting mashup-package. There are 2 benefits of this: 1. The dependency logic is deduplicated. generic builder just concatted lists, whereas all the envs until now would sieve apart haskell and system build inputs. Now, they both decide haskell vs system the same way: according to the argument list and without reflection. Consistency is good, especially because it mean that if the build works, the shell is more likely to work. 2. Cross is handled better. For native builds, because the `ghcWithPackages` calls would shadow, we through both the regular component (lib, exe, test, bench) haskell deps and Setup.hs haskell deps in the same `ghcWithPackages` call. But for cross builds we use `buildPackages.ghcWithPackages` to get the setup deps. This ensures everything works correctly.
We have just a few, and these are regular jobs not must-pass. The tests that were must-pass are left as is.
Ericson2314
force-pushed
the
work-on-multi-shellFor
branch
from
d8e8eba
to
cb46b97
Compare
ofborg
bot
added
8.has: package (new)
10.rebuild-darwin: 11-100
10.rebuild-linux: 11-100
and removed
10.rebuild-darwin: 0
10.rebuild-linux: 0
labels
|
FYI: In I had some code using |
|
I went to comment just on that. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for this change
This makes it work like work-on-multi from Reflex Platform. In
particular, rather than making
.envfromshellFor, we make.envthe primitive, and
shellForworks by combining together the argumentsof all the packages to
generic-builderand taking the.envof theresulting mashup-package.
There are 2 benefits of this:
The dependency logic is deduplicated. generic builder just concatted
lists, whereas all the envs until now would sieve apart haskell and
system build inputs. Now, they both decide haskell vs system the same
way: according to the argument list and without reflection.
Consistency is good, especially because it mean that if the build
works, the shell is more likely to work.
Cross is handled better. For native builds, because the
ghcWithPackagescalls would shadow, we through both the regularcomponent (lib, exe, test, bench) haskell deps and Setup.hs haskell
deps in the same
ghcWithPackagescall. But for cross builds we usebuildPackages.ghcWithPackagesto get the setup deps. This ensureseverything works correctly.
Things done
sandboxinnix.confon non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"./result/bin/)nix path-info -Sbefore and after)Notify maintainers
cc @ElvishJerricco @infinisil