-
Notifications
You must be signed in to change notification settings - Fork 711
cabal new-build
& {alex,happy}
#4009
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
Comments
Turns out, there's a very similiar problem when using legacy sandboxes: haskell/alex#54 |
The following partial revert of 8dc39db was suggested by @dcoutts: diff --git a/cabal-install/Distribution/Client/ProjectBuilding.hs b/cabal-install/Distribution/Client/ProjectBuilding.hs
index 0f3ae1b..0a69c43 100644
--- a/cabal-install/Distribution/Client/ProjectBuilding.hs
+++ b/cabal-install/Distribution/Client/ProjectBuilding.hs
@@ -749,11 +749,11 @@ withTarballLocalDirectory verbosity distDirLayout@DistDirLayout{..}
-- builddir under the tarball src dir to keep path name lengths down.
BuildAndInstall ->
let tmpdir = distTempDirectory in
- withTempDirectory verbosity tmpdir "src" $ \unpackdir ->
- withTempDirectory verbosity tmpdir "build" $ \builddir -> do
+ withTempDirectory verbosity tmpdir "src" $ \unpackdir -> do
unpackPackageTarball verbosity tarball unpackdir
pkgid pkgTextOverride
let srcdir = unpackdir </> display pkgid
+ builddir = srcdir </> "dist"
buildPkg srcdir builddir UPDATE: I've tested the patch above improves the situation |
Packages like `alex` include pre-generated lexer/parser source in their source tarball as e.g. dist/build/alex/alex-tmp/Scan.hs dist/build/alex/alex-tmp/Parser.hs to avoid having to have alex already installed before building alex... Unfortunately, 8dc39db broke packages relying on this accidental feature by changing where the distdir points to. This patch partly reverts that commit and therefore addresses the regression aspect of #4009.
Packages like `alex` include pre-generated lexer/parser source in their source tarball as e.g. dist/build/alex/alex-tmp/Scan.hs dist/build/alex/alex-tmp/Parser.hs to avoid having to have `alex` already installed before building `alex`... Unfortunately, 8dc39db broke packages relying on this accidental feature by changing where the distdir points to. This patch partly reverts that commit and therefore addresses the regression aspect of #4009. This fix was suggested by @dcoutts
OK fixed now. |
@ezyang the |
I can't easily test this but I'll take your word for it. Why doesn't it work for |
Just tested and this appears very much resolved at this point. |
Uh oh!
There was an error while loading. Please reload this page.
UPDATE: Partially (specifically for the case when
alex
/happy
are to be installed into the nix store because they were requested viabuild-tools
), this turns out to be a regression introduced by 8dc39dbnew-build
currently isn't able to self-bootstraphappy
andalex
(which are two special cases in Haskell's ecosystem and for it's imho legitimate to add special logic tonew-build
), and this is causing problems for thenew-build
based Travis scripts which don't installalex
/happy
via binary packages anymore:and similiarly for
alex
:after adding a line
build-tools: happy == 1.19.*
toalex.cabal
, we get now:IMO, it ought to be possible to install
happy
without having a prior version ofhappy
installed, and likewise possible to installalex
without having a prioralex
installed.And in fact,
alex
includes pre-generated.hs
code in its.tar.gz
for that:and similarly
happy
includes:I think it makes sense for
alex
to not specifybuild-tools: alex
, and take that as a hint that there's a pre-generated.x
source bundled in the source-tarball (however, the legacy pathname./dist/build/alex/alex-tmp/Parser.hs
is imho a bad choice, but we will have to support that for the sake of existingalex
/happy
releases). Otoh, if a package specifiesbuild-tools: alex
, I'd expect the.x
files to be regenerated bycabal
(and buildalex
if needed).Maybe for the future, something interacting with the new
autogen-modules
field would make sense here for a more principled solution./cc @ezyang
The text was updated successfully, but these errors were encountered: