Add `--cabal-install` #2995

decentral1se · 2017-02-10T23:51:45Z

Any changes that could be relevant to users have been recorded in the ChangeLog.md
The documentation has been updated, if necessary.

Manually tested.

Based on #2386 (comment). Thanks @sjakobi for the pointers!

sjakobi · 2017-02-11T08:29:06Z

src/Stack/Setup.hs

@@ -140,6 +140,8 @@ data SetupOpts = SetupOpts
    , soptsUpgradeCabal :: !Bool
    -- ^ Upgrade the global Cabal library in the database to the newest
    -- version. Only works reliably with a stack-managed installation.
+    , soptsInstallCabal :: !(Maybe Version)
+    -- ^ Install a specific version of Cabal.


Can you fuse the soptsUpgradeCabal and soptsInstallCabal fields into one? This should make it impossible to have nonsensical states where soptsUpgradeCabal == True and soptsInstallCabal == Just <x>.

Maybe it would even be good to avoid the additional --install-cabal flag and only add an optional argument to --upgrade-cabal?! That would help combating CLI inflation :). Not sure if it's possible with optparse-applicative though…

This sounds like a good idea. I haven't seen an 'optional argument' but will have a look and see what is possible.

Oh, I see that options with optional arguments are not supported but there is a hint to try something else. I think it implies having a default though ... which won't work.

Fusing the two fields and parsing the two options as alternatives would still make sense IMO. As a result the usage info should look roughly like this:

stack setup … [--upgrade-cabal | --install-cabal VERSION] …

The combined field should have a type like Maybe UpgradeTo with

data UpgradeTo = Latest | Specific Version

or so.

The default value (which applies when neither option is used) is Nothing.

BTW, as an example stack clean uses the Alternative instance of the option parser:

Usage: stack clean ([PACKAGE] | [--full]) [--help]

Oh! That was really helpful, thanks for even more pointers :)

sjakobi · 2017-02-11T08:32:28Z

I haven't done a thorough review yet, only added an inline comment on the design so far.

Could you in any case please fix your commit description to use the correct flag name? Cheers! :)

decentral1se · 2017-02-11T09:12:47Z

the correct flag name?

Oh yes, fixed that one 😄

decentral1se · 2017-02-12T13:56:10Z

I've done another pass on this and reduced the diff quite a bit. It should be much easier to review. If there are any other ideas on the merge of --upgrade-cabal/--install-cabal, I'm all ears (see comments above).

decentral1se · 2017-02-18T00:55:19Z

Latest stab at it is c0942c6.

sjakobi

Looks pretty good already but I had a few comments anyway.

sjakobi · 2017-02-18T08:37:55Z

src/Stack/Setup.hs

            $logWarn "This may fail, caveat emptor!"
-        upgradeCabal menv wc
+        upgradeCabal menv wc (fromJust cabalVersion)


It would be nicer to use forM_ instead of when isJust … and fromJust.

Not sure how to get the when isJust out of it? Please see 734dae4.

sjakobi · 2017-02-18T08:43:26Z

src/Stack/Setup.hs

            [] -> error "No Cabal library found in index, cannot upgrade"
-            [PackageIdentifier name' version]
-                | name == name' -> return version
+            [PackageIdentifier name' version] | name == name' -> return version


This block seems unnecessarily monadic, could be pure.

I don't know what this means 😄

The only monadic bit about this case expression is the returns – this should be a let-expression instead of using <-.

sjakobi · 2017-02-18T08:47:20Z

src/Stack/Setup.hs

+            , case cabalVersion of
+                  Specific _ -> " (specified)"
+                  Latest     -> " (latest)"
+            , ". No work to be done."


The string that is being formed here doesn't look right. Anyway, I think we should support installing an older version here. Not sure if that requires adding logic to unregister or hide the old version.

This formatting corresponds to stopping the same version being installed. If you want to get an old version, you just fire off --install-cabal FOO instead of --upgrade-cabal. Good question regarding the unregister/hide stuff. Will investigate.

Examples are:

λ :main ["setup", "--upgrade-cabal"] Currently installed Cabal is 1.24.2.0 (latest). No work to be done.

And:

λ :main ["setup", "--install-cabal", "1.24.2.0"] Currently installed Cabal is 1.24.2.0 (specified). No work to be done.

Ok, I think I misread the diff a bit. Still, even when version < installed, we want to setup that old version of Cabal.

sjakobi · 2017-02-18T08:48:56Z

src/Stack/Setup.hs

            -- Nothing below: use the newest .cabal file revision
            m <- unpackPackageIdents menv tmpdir Nothing (Map.singleton ident Nothing)

            compilerPath <- join $ findExecutable menv (compilerExeName wc)
-            newestDir <- parseRelDir $ versionString newest
+            newestDir <- parseRelDir $ versionString version


For consistency, that should probably be "versionDir".

sjakobi · 2017-02-18T08:51:24Z

src/Stack/Types/Version.hs

@@ -64,6 +65,9 @@ instance Exception VersionParseFail
 instance Show VersionParseFail where
    show (VersionParseFail bs) = "Invalid version: " ++ show bs

+-- | A package version or the latest.
+data UpgradeTo = Specific Version | Latest deriving (Show)
+


The description is way more general than the name of the type. Can you come up with a more general name maybe?

Hmmm ... tried the description again. I like the name, it was your recommendation :)

sjakobi · 2017-02-22T01:34:21Z

src/Stack/Setup.hs

@@ -494,7 +494,7 @@ ensureCompiler sopts = do
        unless needLocal $ do
            $logWarn "Trying to change a Cabal library on a GHC not installed by stack."
            $logWarn "This may fail, caveat emptor!"
-        upgradeCabal menv wc (fromJust cabalVersion)
+        forM_ cabalVersion (upgradeCabal menv wc)


No, the forM_ should wrap the entire block:

forM_ cabalVersion $ \v -> do unless …

mgsloan · 2017-02-22T05:56:42Z

Looks like good stuff! I certainly recommend making sure the downgrade case works. I'd also have expected it to require unregistering the old one.

snoyberg · 2017-02-22T17:19:55Z

Moving over the discussion from #3019:

Upgrading the global Cabal package is an ugly hack that I added in towards the beginning of Stack. It was necessary for cases where a custom setup script required some new functionality in Cabal that was not present in the shipped-with-GHC version. However, since then, we've had a significant change: custom-setup stanzas. With these, we're no longer trapped by random whims of the package database, and trying to guess what set of packages should be available to Setup.hs. In particular:

Stackage snapshots will now build packages reliably against the snapshot version of Cabal
If a specific Cabal version is needed, it can be specified in a .cabal file
Instead of being pinned to building against the global version of Cabal, we can build against the snapshot version

So I'd like to go in almost the opposite direction of this PR (apologies to @lwm and @sjakobi for only commenting now):

Get rid of the hack of upgrading global Cabal versions at all
Give massive warnings if a custom setup script does not specify its dependencies via custom-setup

I'd go so far as considering upstream—in Stackage itself—beginning to block packages that don't define a custom-setup stanza.

sjakobi · 2017-02-22T18:10:27Z

@snoyberg Interesting!

So to check that stack is compatible with Setup.hs files built with several versions of Cabal I'd simply use a different resolver (see e.g. #2350 for a usecase)?
Will overriding the resolver's version of Cabal via extra-deps or packages be supported?

snoyberg · 2017-02-22T18:36:34Z

I'm pretty sure that overriding the Cabal version will work with master, though to be honest I haven't tested it explicitly.

sjakobi · 2017-02-22T19:46:39Z

@snoyberg

I'm pretty sure that overriding the Cabal version will work with master

Not sure if I'm missing something but it doesn't seem to work:

~/tmp $ stack --resolver lts-8.2 new cabal-setup
~/tmp $ cd cabal-setup/
~/t/cabal-setup $ vim stack.yaml # Add Cabal-1.24.0.0 to extra-deps
~/t/cabal-setup $ stack build
Warning: File listed in cabal-setup.cabal file does not exist: README.md
cabal-setup-0.1.0.0: configure (lib + exe)
Configuring cabal-setup-0.1.0.0...
cabal-setup-0.1.0.0: build (lib + exe)
Preprocessing library cabal-setup-0.1.0.0...
[1 of 1] Compiling Lib              ( src/Lib.hs, .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/Lib.o )
Preprocessing executable 'cabal-setup-exe' for cabal-setup-0.1.0.0...
[1 of 1] Compiling Main             ( app/Main.hs, .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/cabal-setup-exe/cabal-setup-exe-tmp/Main.o )
Linking .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/cabal-setup-exe/cabal-setup-exe ...
Warning: File listed in cabal-setup.cabal file does not exist: README.md
cabal-setup-0.1.0.0: copy/register
Installing library in
/home/simon/tmp/cabal-setup/.stack-work/install/x86_64-linux/lts-8.2/8.0.2/lib/x86_64-linux-ghc-8.0.2/cabal-setup-0.1.0.0-Bd2jGUn5HBKJ5lzwApT2Rh
Installing executable(s) in
/home/simon/tmp/cabal-setup/.stack-work/install/x86_64-linux/lts-8.2/8.0.2/bin
Registering cabal-setup-0.1.0.0...

Judging from the build paths stack used Cabal-1.24.2.0 instead of 1.24.0.0 to build the Setup.hs.

snoyberg · 2017-02-22T19:48:23Z

Is this using custom setup at all?

…

On Wed, Feb 22, 2017, 9:46 PM Simon Jakobi ***@***.***> wrote: @snoyberg <https://github.com/snoyberg> I'm pretty sure that overriding the Cabal version will work with master Not sure if I'm missing something but it doesn't seem to work: ~/tmp $ stack --resolver lts-8.2 new cabal-setup ~/tmp $ cd cabal-setup/ ~/t/cabal-setup $ vim stack.yaml # Add Cabal-1.24.0.0 to extra-deps ~/t/cabal-setup $ stack build Warning: File listed in cabal-setup.cabal file does not exist: README.md cabal-setup-0.1.0.0: configure (lib + exe) Configuring cabal-setup-0.1.0.0... cabal-setup-0.1.0.0: build (lib + exe) Preprocessing library cabal-setup-0.1.0.0... [1 of 1] Compiling Lib ( src/Lib.hs, .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/Lib.o ) Preprocessing executable 'cabal-setup-exe' for cabal-setup-0.1.0.0... [1 of 1] Compiling Main ( app/Main.hs, .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/cabal-setup-exe/cabal-setup-exe-tmp/Main.o ) Linking .stack-work/dist/x86_64-linux/Cabal-1.24.2.0/build/cabal-setup-exe/cabal-setup-exe ... Warning: File listed in cabal-setup.cabal file does not exist: README.md cabal-setup-0.1.0.0: copy/register Installing library in /home/simon/tmp/cabal-setup/.stack-work/install/x86_64-linux/lts-8.2/8.0.2/lib/x86_64-linux-ghc-8.0.2/cabal-setup-0.1.0.0-Bd2jGUn5HBKJ5lzwApT2Rh Installing executable(s) in /home/simon/tmp/cabal-setup/.stack-work/install/x86_64-linux/lts-8.2/8.0.2/bin Registering cabal-setup-0.1.0.0... Judging from the build paths stack used Cabal-1.24.2.0 instead of 1.24.0.0 to build the Setup.hs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2995 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB0SGLB3SQ7u-uEFENmZKfVoD2ADPks5rfJCkgaJpZM4L9-vq> .

sjakobi · 2017-02-22T19:51:17Z

Is this using custom setup at all?

No, but should that be a requirement for testing compatibility between stack and Cabal?

snoyberg · 2017-02-22T19:53:55Z

Ah, I hadn't understood that this is the case you're trying to address. I guess this feature makes sense in that context. Ignore my objections.

…

On Wed, Feb 22, 2017, 9:51 PM Simon Jakobi ***@***.***> wrote: Is this using custom setup at all? No, but should that be a requirement for testing compatibility between stack and Cabal? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2995 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB_XgKOb9PE4toeKFiDjMJvPb4Hlsks5rfJG4gaJpZM4L9-vq> .

sjakobi · 2017-02-22T20:22:15Z

Ah, good! Maybe the usage info for --upgrade-cabal and install-cabal should note that these flags are only for debugging now.

snoyberg · 2017-02-22T20:27:00Z

SGTM 👍

…

On Wed, Feb 22, 2017, 10:22 PM Simon Jakobi ***@***.***> wrote: Ah, good! Maybe the usage info for --upgrade-cabal and install-cabal should note that these flags are only for debugging now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2995 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADBB6GW8RGHc9dalkzXzocYNMHVpsPOks5rfJj6gaJpZM4L9-vq> .

decentral1se · 2017-03-08T14:46:48Z

Kind of fell off on this one. Is this PR still relevant? I see lots of movement around Cabal stuff.

mgsloan · 2017-03-16T22:40:39Z

@lwm I think the main unresolved issue is handling the downgrade case. Currently with this patch, if I have Cabal-1.24.2.0 and run stack setup --install-cabal 1.24.0.0, I get Currently installed Cabal is 1.24.2.0 (specified). No work to be done.

I think this is still relevant. I don't see how custom-setup obviates the need for this, since the global cabal is still used for everything else.

decentral1se · 2017-03-19T14:06:20Z

Thanks for feedback @mgsloan and @sjakobi.

I've given this another stab. Apologies that the diff on this latest patch diverged so far but I tried a new approach. It appears the downgrade case is covered now but it takes so long to get a cabal install on my crappy machine, I didn't spend long testing ;)

decentral1se · 2017-03-19T21:49:26Z

Failure unrelated ... tests seem to be borked:

src/test/Stack/ConfigSpec.hs:154:22:
    No instance for (FromJSON (WithJSONWarnings ConfigMonoid))
      arising from a use of ‘decodeEither’
    In the expression: decodeEither defaultConfigYaml
    In an equation for ‘parsed’:
        parsed = decodeEither defaultConfigYaml
    In the expression:
      do { let parsed :: Either String (WithJSONWarnings ConfigMonoid)
               parsed = decodeEither defaultConfigYaml;
           isRight parsed `shouldBe` True }

mgsloan · 2017-03-19T22:23:42Z

@lwm My mistake, sorry! Should be fixed after rebasing.

decentral1se · 2017-03-20T12:01:55Z

No worries @mgsloan! Travis said OK but now Appveyor said:

C:\stack\src\test\Stack\ConfigSpec.hs:154:53: error:
    * Exception when trying to run compile-time code:
        InvalidAbsDir "/"
CallStack (from HasCallStack):
  error, called at src/Path.hs:242:17 in path-0.5.12-DPixZY6jeMPCLEenb9rPGQ:Path
      Code: mkAbsDir "/"
    * In the untyped splice: $(mkAbsDir "/")
--  While building package stack-1.4.1 using:

The never ending pull request lives on!

mgsloan · 2017-03-20T22:15:41Z

Oops, fixed that too. I'm going to go ahead and merge this. Thanks!!

sjakobi reviewed Feb 11, 2017

View reviewed changes

sjakobi requested changes Feb 18, 2017

View reviewed changes

sjakobi reviewed Feb 22, 2017

View reviewed changes

decentral1se mentioned this pull request Feb 22, 2017

Remove --upgrade-cabal recommendation #3019

Merged

mgsloan mentioned this pull request Mar 8, 2017

Remove special global Cabal? #3049

Closed

Add --install-cabal option for stack setup.

90ae9ec

mgsloan merged commit 5f7c0f2 into commercialhaskell:master Mar 20, 2017

decentral1se deleted the add-cabal-install branch March 20, 2017 22:20

decentral1se mentioned this pull request Mar 30, 2017

Add stack setup --install-cabal VERSION in addition to stack setup --upgrade-cabal. #2386

Closed

Add --cabal-install #2995

Add --cabal-install #2995

Conversation

decentral1se commented Feb 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentral1se Feb 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sjakobi commented Feb 11, 2017

decentral1se commented Feb 11, 2017 • edited Loading

decentral1se commented Feb 12, 2017

decentral1se commented Feb 18, 2017

sjakobi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentral1se Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

decentral1se Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgsloan commented Feb 22, 2017

snoyberg commented Feb 22, 2017

sjakobi commented Feb 22, 2017

snoyberg commented Feb 22, 2017

sjakobi commented Feb 22, 2017

snoyberg commented Feb 22, 2017 via email

sjakobi commented Feb 22, 2017

snoyberg commented Feb 22, 2017 via email

sjakobi commented Feb 22, 2017

snoyberg commented Feb 22, 2017 via email

decentral1se commented Mar 8, 2017

mgsloan commented Mar 16, 2017

decentral1se commented Mar 19, 2017

decentral1se commented Mar 19, 2017

mgsloan commented Mar 19, 2017

decentral1se commented Mar 20, 2017

mgsloan commented Mar 20, 2017

Add `--cabal-install` #2995

Add `--cabal-install` #2995

decentral1se Feb 12, 2017 •

edited

Loading

decentral1se commented Feb 11, 2017 •

edited

Loading

decentral1se Feb 19, 2017 •

edited

Loading

decentral1se Feb 19, 2017 •

edited

Loading