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

support cabal-doctest #427

Merged
merged 23 commits into from
Mar 20, 2021
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5f9bdc5
Add test.
cdepillabout Feb 5, 2020
f7dc472
wip
cdepillabout Jan 31, 2020
a1f6eaf
Merge branch 'master' into support-cabal-doctest
angerman Feb 18, 2020
c7ba0f9
Merge branch 'master' into support-cabal-doctest
hamishmack Mar 4, 2020
1d8a687
Merge master into support-cabal-doctest
hamishmack Mar 21, 2020
473c375
Fix merge regression
hamishmack Mar 21, 2020
c659d88
Combine drv, drv.source and drv.dist for doctest
hamishmack Mar 21, 2020
fa9597f
Skip cabal-doctests test when cross compiling
hamishmack Mar 21, 2020
1d6dd9c
Add the --no-magic flag to the .cabal file for the cabal-doctests test.
cdepillabout Mar 30, 2020
c3e883c
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Apr 16, 2020
9caf1cd
Merge branch 'master' into support-cabal-doctest
angerman May 1, 2020
cb20a5a
Merge master into support-cabal-doctest
hamishmack Jan 14, 2021
1b2eae0
Fix cabal-doctest support
hamishmack Jan 15, 2021
69ba0c1
Skip cabal-doctest test plan cross compiling
hamishmack Jan 15, 2021
da71b82
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Jan 25, 2021
cc58520
More fixes for cabal-doctest
hamishmack Jan 25, 2021
506e63a
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Jan 25, 2021
37c662a
Skip cabal-doctest tests when cross compiling
hamishmack Jan 25, 2021
49aca8b
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Feb 15, 2021
7bb7914
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Feb 16, 2021
49a78b8
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Mar 18, 2021
54486bb
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Mar 18, 2021
8d3cd2e
Merge remote-tracking branch 'origin/master' into support-cabal-doctest
hamishmack Mar 19, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
62 changes: 51 additions & 11 deletions builder/comp-builder.nix
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,14 @@ let self =
, preInstall ? component.preInstall , postInstall ? component.postInstall
, preHaddock ? component.preHaddock , postHaddock ? component.postHaddock
, shellHook ? ""
, configureAllComponents ? component.configureAllComponents ||
# When set, configure all the components in the package
# (not just the one we are building).
# Enable for tests in packages that use cabal-doctest.
( haskellLib.isTest componentId &&
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends
Copy link
Contributor

Choose a reason for hiding this comment

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

I think package.setup-depends is wrong here. If a single test-suite depends on cabal-doctest then setup-depends will contain cabal-doctest. But if another test-suite is defined that doesn't depend on cabal-doctest, then this test-suite will also build all components. I think this is wrong. Can we look at just what this component depends on?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here's my humble suggestion:

Suggested change
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends
lib.any (x: x.identifier.name or "" == "cabal-doctest") package.setup-depends &&
lib.any (x: x.identifier.name or """ == "doctest") component.depends

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dumb suggestion: what if we just made people set this themselves? This is only a problem for test components using doctest, so is it totally unreasonable to say "if you use doctest, you should set this option on the corresponding test component"?

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we could, but I like the mantra of "if you do cabal X and it works, then it will work with Haskell.nix". That will no longer hold for this case now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I have a feeling I'm about to say something I've said in the past, but... why does this work in cabal itself? I vaguely recall that cabal turns off per-component builds if you have a custom setup - should we do the same thing? Maybe that would work more simply than trying to detect cabal-doctest?

Copy link
Contributor

Choose a reason for hiding this comment

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

That one is outside my knowledge, I'm afraid!

Copy link
Collaborator

Choose a reason for hiding this comment

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

should we do the same thing?

I think this change is a step in that direction.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it works well enough to use for all custom setup packages yet. In particular I think the component filtering will not work.

)
, allComponent # Used when `configureAllComponents` is set to get a suitable configuration.

, build-tools ? component.build-tools
, pkgconfig ? component.pkgconfig
Expand All @@ -45,7 +53,7 @@ let self =
, doHoogle ? component.doHoogle # Also build a hoogle index
, hyperlinkSource ? component.doHyperlinkSource # Link documentation to the source code
, quickjump ? component.doQuickjump # Generate an index for interactive documentation navigation
, keepSource ? component.keepSource # Build from `source` output in the store then delete `dist`
, keepSource ? component.keepSource || configureAllComponents # Build from `source` output in the store then delete `dist`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why?

, setupHaddockFlags ? component.setupHaddockFlags

# Profiling
Expand Down Expand Up @@ -75,6 +83,11 @@ let self =
}@drvArgs:

let
componentForSetup =
if configureAllComponents
then allComponent
else component;

# TODO fix cabal wildcard support so hpack wildcards can be mapped to cabal wildcards
canCleanSource = !(cabal-generator == "hpack" && !(package.cleanHpack or false));
# In order to support relative references to other packages we need to use
Expand All @@ -84,7 +97,7 @@ let
# is the sub directory in that root path that contains the package.
# `cleanSrc.subDir` is used in `prePatch` and `lib/cover.nix`.
cleanSrc = haskellLib.rootAndSubDir (if canCleanSource
then haskellLib.cleanCabalComponent package component "${componentId.ctype}-${componentId.cname}" src
then haskellLib.cleanCabalComponent package componentForSetup "${componentId.ctype}-${componentId.cname}" src
else
# We can clean out the siblings though to at least avoid changes to other packages
# from triggering a rebuild of this one.
Expand All @@ -106,8 +119,9 @@ let
needsProfiling = enableExecutableProfiling || enableLibraryProfiling;

configFiles = makeConfigFiles {
component = componentForSetup;
inherit (package) identifier;
inherit component fullName flags needsProfiling;
inherit fullName flags needsProfiling;
};

enableFeature = enable: feature:
Expand All @@ -117,8 +131,13 @@ let

finalConfigureFlags = lib.concatStringsSep " " (
[ "--prefix=$out"
"${haskellLib.componentTarget componentId}"
"$(cat ${configFiles}/configure-flags)"
] ++ (
# If configureAllComponents is set we should not specify the component
# and Setup will attempt to configure them all.
if configureAllComponents
then ["--enable-tests" "--enable-benchmarks"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this respect package/component settings of whether to enable tests/benchmarks?

else ["${haskellLib.componentTarget componentId}"]
) ++ [ "$(cat ${configFiles}/configure-flags)"
] ++ commonConfigureFlags);

# From nixpkgs 20.09, the pkg-config exe has a prefix matching the ghc one
Expand Down Expand Up @@ -316,15 +335,17 @@ let
++ (lib.optional enableSeparateDataOutput "data")
++ (lib.optional keepSource "source");

configurePhase =
prePatch =
(lib.optionalString (!canCleanSource) ''
echo "Cleaning component source not supported, leaving it un-cleaned"
'') +
(lib.optionalString keepSource ''
cp -r . $source
cd $source
chmod -R +w .
'') + ''
'') + commonAttrs.prePatch;

configurePhase = ''
runHook preConfigure
echo Configure flags:
printf "%q " ${finalConfigureFlags}
Expand Down Expand Up @@ -352,7 +373,11 @@ let
target-pkg-and-db = "${ghc.targetPrefix}ghc-pkg -v0 --package-db $out/package.conf.d";
in ''
runHook preInstall
$SETUP_HS copy ${lib.concatStringsSep " " setupInstallFlags}
$SETUP_HS copy ${lib.concatStringsSep " " (
setupInstallFlags
++ lib.optional configureAllComponents
(haskellLib.componentTarget componentId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we just do this unconditionally? Does it hurt us to setup copy a specific component even if we just configured for that component?

)}
${lib.optionalString (haskellLib.isLibrary componentId) ''
$SETUP_HS register --gen-pkg-config=${name}.conf
${ghc.targetPrefix}ghc-pkg -v0 init $out/package.conf.d
Expand Down Expand Up @@ -439,9 +464,24 @@ let
'')
}
runHook postInstall
'' + (lib.optionalString keepSource ''
rm -rf dist
'') + (lib.optionalString (haskellLib.isTest componentId) ''
'' + (
# Keep just the autogen files and package.conf.inplace package
# DB (probably empty unless this is a library component).
# We also have to remove any refernces to $out to avoid
# circular references.
if configureAllComponents
then ''
mv dist dist-tmp-dir
mkdir -p dist/build
mv dist-tmp-dir/build/${componentId.cname}/autogen dist/build/
mv dist-tmp-dir/package.conf.inplace dist/
remove-references-to -t $out dist/build/autogen/*
rm -rf dist-tmp-dir
''
else lib.optionalString keepSource ''
rm -rf dist
''
) + (lib.optionalString (haskellLib.isTest componentId) ''
echo The test ${package.identifier.name}.components.tests.${componentId.cname} was built. To run the test build ${package.identifier.name}.checks.${componentId.cname}.
'');

Expand Down
6 changes: 3 additions & 3 deletions builder/hspkg-builder.nix
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,14 @@ let
inherit (pkg) preUnpack postUnpack;
};

buildComp = componentId: component: comp-builder {
inherit componentId component package name src flags setup cabalFile cabal-generator patches revision
buildComp = allComponent: componentId: component: comp-builder {
inherit allComponent componentId component package name src flags setup cabalFile cabal-generator patches revision
shellHook
;
};

in rec {
components = haskellLib.applyComponents buildComp pkg;
components = haskellLib.applyComponents (buildComp pkg.allComponent) pkg;
checks = pkgs.recurseIntoAttrs (builtins.mapAttrs
(_: d: haskellLib.check d)
(lib.filterAttrs (_: d: d.config.doCheck) components.tests));
Expand Down
2 changes: 1 addition & 1 deletion lib/check.nix
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ let
in stdenv.mkDerivation ({
name = (drv.name + "-check");

# Useing `srcOnly` (rather than getting the `src` via a `drv.passthru`)
# Using `srcOnly` (rather than getting the `src` via a `drv.passthru`)
# should correctly apply the patches from `drv` (if any).
src = drv.source or (srcOnly drv);

Expand Down
199 changes: 119 additions & 80 deletions modules/package.nix
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,86 @@ with types;

# Work around issue that can cause _lots_ of files to be copied into the store.
# See https://github.com/NixOS/nixpkgs/pull/64691
let path = types.path // { check = x: types.path.check (x.origSrc or x); };
let
path = types.path // { check = x: types.path.check (x.origSrc or x); };

componentType = submodule {
# add the shared componentOptions
options = (packageOptions config) // {
buildable = mkOption {
type = bool;
default = true;
};
depends = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
libs = mkOption {
type = listOfFilteringNulls (nullOr package);
default = [];
};
frameworks = mkOption {
type = listOfFilteringNulls package;
default = [];
};
pkgconfig = mkOption {
type = listOf (listOfFilteringNulls package);
default = [];
};
build-tools = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
modules = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
asmSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cmmSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cxxSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
jsSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
hsSourceDirs = mkOption {
type = listOfFilteringNulls unspecified;
default = ["."];
};
includeDirs = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
includes = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
mainPath = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
extraSrcFiles = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
platforms = mkOption {
type = nullOr (listOfFilteringNulls unspecified);
default = null;
};
};
};

in {
# This is how the Nix expressions generated by *-to-nix receive
Expand Down Expand Up @@ -143,85 +222,7 @@ in {
};
};

components = let
componentType = submodule {
# add the shared componentOptions
options = (packageOptions config) // {
buildable = mkOption {
type = bool;
default = true;
};
depends = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
libs = mkOption {
type = listOfFilteringNulls (nullOr package);
default = [];
};
frameworks = mkOption {
type = listOfFilteringNulls package;
default = [];
};
pkgconfig = mkOption {
type = listOf (listOfFilteringNulls package);
default = [];
};
build-tools = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
modules = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
asmSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cmmSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
cxxSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
jsSources = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
hsSourceDirs = mkOption {
type = listOfFilteringNulls unspecified;
default = ["."];
};
includeDirs = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
includes = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
mainPath = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
extraSrcFiles = mkOption {
type = listOfFilteringNulls unspecified;
default = [];
};
platforms = mkOption {
type = nullOr (listOfFilteringNulls unspecified);
default = null;
};
};
};
in {
components = {
setup = mkOption {
type = nullOr componentType;
default = {
Expand Down Expand Up @@ -300,5 +301,43 @@ in {
type = listOf (either unspecified path);
default = [];
};
# This used to be `components.all` but it has been added back as `allComponent` to
# to avoid confusion. It is not mapped by `builder/hspkg-builder.nix` to anything
# you can build. Instead it is used internally when `configureAllComponents`
# is set or for tests whe on `cabal-doctest` is in the `setup-depends` of the package.
allComponent = mkOption {
Copy link
Collaborator

Choose a reason for hiding this comment

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

The villain returns :p

type = componentType;
apply = all: all // {
# TODO: Should this check for the entire component
# definition to match, rather than just the identifier?
depends = builtins.filter (p: p.identifier != config.package.identifier) all.depends;
};
description = "The merged dependencies of all other components";
};
};

# This has one quirk. Manually setting options on the all component
# will be considered a conflict. This is almost always fine; most
# settings should be modified in either the package options, or an
# individual component's options. When this isn't sufficient,
# mkForce is a reasonable workaround.
#
# An alternative solution to mkForce for many of the options where
# this is relevant would be to switch from the bool type to
# something like an anyBool type, which would merge definitions by
# returning true if any is true.
config.allComponent =
let allComps = haskellLib.getAllComponents config;
in lib.mkMerge (
builtins.map (c:
# Exclude attributes that are likely to have conflicting definitions
# (a common use case for `all` is in `shellFor` and it only has an
# install phase).
builtins.removeAttrs c ["preCheck" "postCheck" "keepSource"]
) allComps
) // {
# If any one of the components needs us to keep the source
# then keep it for the `all` component
keepSource = lib.foldl' (x: comp: x || comp.keepSource) false allComps;
};
}
5 changes: 5 additions & 0 deletions modules/plan.nix
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,11 @@ let
type = bool;
default = (def.enableShared or true);
};
configureAllComponents = mkOption {
description = "If set all the components in the package are configured (useful for cabal-doctest).";
type = bool;
default = false;
};
shellHook = mkOption {
description = "Hook to run when entering a shell";
type = unspecified; # Can be either a string or a function
Expand Down
6 changes: 6 additions & 0 deletions test/cabal-doctests/Setup.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module Main where

import Distribution.Extra.Doctest (defaultMainWithDoctests)

main :: IO ()
main = defaultMainWithDoctests "doctests"