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

Have Cabal build a Setup executable and test with it. #3260

Closed
wants to merge 1 commit into from

Conversation

ezyang
Copy link
Contributor

@ezyang ezyang commented Mar 31, 2016

I recently discovered that build-tools can be used to specify
dependencies on internal executables (#220). This means that
we can make a nice improvement to the package-tests test suite:
instead of manually building a Setup.hs script ourselves,
we can add one to the Cabal file for Cabal, and have the
test suite have a build-tools dependency on it.

Perhaps the only objection to this is that the Cabal library
now has an executable cabal-setup. Maybe with some buildable
shenanigans we can make it so that we never build this executable
unless the user asks for it, or the test suite is enabled.
Alternately, cabal-setup is a generally handy executable to have
around.

Signed-off-by: Edward Z. Yang [email protected]

I recently discovered that build-tools can be used to specify
dependencies on internal executables (haskell#220).  This means that
we can make a nice improvement to the package-tests test suite:
instead of manually building a Setup.hs script ourselves,
we can add one to the Cabal file for Cabal, and have the
test suite have a build-tools dependency on it.

Perhaps the only objection to this is that the Cabal *library*
now has an executable cabal-setup.  Maybe with some buildable
shenanigans we can make it so that we never build this executable
unless the user asks for it, or the test suite is enabled.
Alternately, cabal-setup is a generally handy executable to have
around.

Signed-off-by: Edward Z. Yang <[email protected]>
@@ -342,6 +342,14 @@ library
if !impl(ghc >= 7.0)
default-extensions: CPP

executable cabal-setup
Copy link
Member

Choose a reason for hiding this comment

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

This means that cabal install Cabal will now also install cabal-setup. Would be nice to avoid that, perhaps by building cabal-setup only when a flag is enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Still testing, but how does something like this look:

diff --git a/Cabal/Cabal.cabal b/Cabal/Cabal.cabal
index 494eb97..b182bc6 100644
--- a/Cabal/Cabal.cabal
+++ b/Cabal/Cabal.cabal
@@ -212,6 +212,9 @@ source-repository head
 flag bundled-binary-generic
   default: False

+flag cabal-setup-buildable
+  default: False
+
 library
   build-depends:
     array      >= 0.1 && < 0.6,
@@ -347,6 +350,10 @@ library
     default-extensions: CPP

 executable cabal-setup
+  if flag(cabal-setup-buildable)
+    buildable: True
+  else
+    buildable: False
   build-depends: base, Cabal
   main-is: Setup.hs
   -- Actually, we don't really want anything from tests; just
@@ -392,7 +399,11 @@ test-suite package-tests
     PackageTests.TestSuiteTests.ExeV10.Check
     PackageTests.PackageTester
   hs-source-dirs: tests
-  build-tools: cabal-setup
+  if flag(cabal-setup-buildable)
+    buildable: True
+    build-tools: cabal-setup
+  else
+    buildable: False
   build-depends:
     base,
     containers,

Copy link
Member

Choose a reason for hiding this comment

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

Yep, that's what I had I mind. Is Cabal smart enough to see that --enable-tests must imply cabal-setup-buildable: True? Then maybe the second conditional could be elided.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not, because build-tools are not considered during dependency resolution; this is bug #220

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, this interacts poorly with new-build, now it thinks that the package has no component named package-tests. Reported in #3262.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 1, 2016

OK, well, this patch doesn't simplify that much, so I guess if the added executable is a deal breaker lets not put it in.

@ezyang
Copy link
Contributor Author

ezyang commented Apr 5, 2016

Canned.

@ezyang ezyang closed this Apr 5, 2016
@ezyang ezyang deleted the cabal-setup branch April 8, 2016 02:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants