-
Notifications
You must be signed in to change notification settings - Fork 247
support cabal-doctest #427
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
Changes from all commits
5f9bdc5
f7dc472
a1f6eaf
c7ba0f9
1d8a687
473c375
c659d88
fa9597f
1d6dd9c
c3e883c
9caf1cd
cb20a5a
1b2eae0
69ba0c1
da71b82
cc58520
506e63a
37c662a
49aca8b
7bb7914
49a78b8
54486bb
8d3cd2e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
) | ||
, allComponent # Used when `configureAllComponents` is set to get a suitable configuration. | ||
|
||
, build-tools ? component.build-tools | ||
, pkgconfig ? component.pkgconfig | ||
|
@@ -47,7 +55,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` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? |
||
, setupHaddockFlags ? component.setupHaddockFlags | ||
|
||
# Profiling | ||
|
@@ -77,6 +85,11 @@ let self = | |
}@drvArgs: | ||
|
||
let | ||
componentForSetup = | ||
if configureAllComponents | ||
then allComponent | ||
else component; | ||
|
||
# Ignore attempts to include DWARF info when it is not possible | ||
enableDWARF = drvArgs.enableDWARF or false | ||
&& stdenv.hostPlatform.isLinux | ||
|
@@ -96,7 +109,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. | ||
|
@@ -118,8 +131,9 @@ let | |
needsProfiling = enableExecutableProfiling || enableLibraryProfiling; | ||
|
||
configFiles = makeConfigFiles { | ||
component = componentForSetup; | ||
inherit (package) identifier; | ||
inherit component fullName flags needsProfiling enableDWARF; | ||
inherit fullName flags needsProfiling enableDWARF; | ||
}; | ||
|
||
enableFeature = enable: feature: | ||
|
@@ -129,8 +143,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"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -328,7 +347,7 @@ let | |
++ (lib.optional enableSeparateDataOutput "data") | ||
++ (lib.optional keepSource "source"); | ||
|
||
configurePhase = | ||
prePatch = | ||
# emcc is very slow if it cannot cache stuff in $HOME | ||
(lib.optionalString (stdenv.hostPlatform.isGhcjs) '' | ||
export HOME=$(mktemp -d) | ||
|
@@ -340,7 +359,9 @@ let | |
cp -r . $source | ||
cd $source | ||
chmod -R +w . | ||
'') + '' | ||
'') + commonAttrs.prePatch; | ||
|
||
configurePhase = '' | ||
runHook preConfigure | ||
echo Configure flags: | ||
printf "%q " ${finalConfigureFlags} | ||
|
@@ -368,7 +389,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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we just do this unconditionally? Does it hurt us to |
||
)} | ||
${lib.optionalString (haskellLib.isLibrary componentId) '' | ||
$SETUP_HS register --gen-pkg-config=${name}.conf | ||
${ghc.targetPrefix}ghc-pkg -v0 init $out/package.conf.d | ||
|
@@ -455,9 +480,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}. | ||
''); | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -138,85 +217,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 = { | ||
|
@@ -295,5 +296,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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
}; | ||
} |
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" |
There was a problem hiding this comment.
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 singletest-suite
depends oncabal-doctest
thensetup-depends
will containcabal-doctest
. But if anothertest-suite
is defined that doesn't depend oncabal-doctest
, then thistest-suite
will also build all components. I think this is wrong. Can we look at just what this component depends on?There was a problem hiding this comment.
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:
There was a problem hiding this comment.
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"?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
?There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this change is a step in that direction.
There was a problem hiding this comment.
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.