Implement the '--allow-newer' option for 'configure'/'install' #1604

Merged
merged 17 commits into from Dec 9, 2013

Conversation

Projects
None yet
3 participants
@23Skidoo
Member

23Skidoo commented Dec 5, 2013

Fixes #949. Example usage:

$ grep array tst.cabal
  build-depends:       base >=4.5 && <4.6, array >= 0.3 && < 0.4

$ ghc-pkg list array --simple-output
array-0.4.0.0

$ cabal configure
Resolving dependencies...
cabal: Could not resolve dependencies:
trying: tst-0.1.0.0 (user goal)
next goal: array (dependency of tst-0.1.0.0)
rejecting: array-0.4.0.0/installed-4f6... (conflict: tst => array>=0.3 &&
<0.4)
Dependency tree exhaustively searched.

$ cabal configure --allow-newer
Resolving dependencies...
Configuring tst-0.1.0.0...

$ cabal build
Building tst-0.1.0.0...
Preprocessing executable 'tst' for tst-0.1.0.0...
[1 of 1] Compiling Main             ( Main.hs, dist/build/tst/tst-tmp/Main.o )
Linking dist/build/tst/tst ...

Known issues:

  • Dependencies on internal libraries are not relaxed.
  • build-depends: a < 3 && >= 2; if (someFlag): build-depends: a >= 5 && < 6 gets relaxed to build-depends: >= 2; if (someFlag): build-depends: a >= 5 (4 is now allowed where it previously wasn't).
  • When configure --exact-configuration fails, the error message should probably mention --exact-configuration.

23Skidoo added some commits Dec 1, 2013

Solver support for '--allow-newer'.
Implemented by going through all packages in the 'depResolverSourcePkgIndex' and
applying 'relaxUpperBound' to the dependencies listed in 'build-depends'.

Known issue:
'build-depends: a < 3 && >= 2; if (someFlag): build-depends: a >= 5 && < 6'
gets converted to
'build-depends: >= 2; if (someFlag): build-depends: a >= 5'
(4 is now allowed where it previously wasn't).

Example:

    $ cabal install --dry-run ./tst
    Resolving dependencies...
    In order, the following would be installed (use -v for more details):
    array-0.3.0.3 (latest: 0.5.0.0)
    tst-0.1.0.0 (latest: 0.1.1)

    $ cabal install --dry-run --allow-newer=array ./tst
    Resolving dependencies...
    In order, the following would be installed (use -v for more details):
    tst-0.1.0.0 (latest: 0.1.1)
@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

I think this would be better named as removeUpperBound. Otherwise fine.

dcoutts commented on 7d0cefe Dec 6, 2013

I think this would be better named as removeUpperBound. Otherwise fine.

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

Fixed.

Owner

23Skidoo replied Dec 6, 2013

Fixed.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Fine.

dcoutts commented on 9a5b601 Dec 6, 2013

Fine.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Nice and modular.

So the strategy is ok for now. We should ask Andres if he can think about how to solve the problem with flags and allowing known-bad versions.

dcoutts commented on d5ea0df Dec 6, 2013

Nice and modular.

So the strategy is ok for now. We should ask Andres if he can think about how to solve the problem with flags and allowing known-bad versions.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

If we just have it in the configure ex and not in install flags then we don't have this overlap here.

If we just have it in the configure ex and not in install flags then we don't have this overlap here.

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

Fixed.

Owner

23Skidoo replied Dec 6, 2013

Fixed.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Ok, if allow-newer is in ConfigExFlags then we don't need it in InstallFlags right? Notice that you're having to filter it out.

dcoutts commented on 11f37ac Dec 6, 2013

Ok, if allow-newer is in ConfigExFlags then we don't need it in InstallFlags right? Notice that you're having to filter it out.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Yep.

dcoutts commented on 5b12ba0 Dec 6, 2013

Yep.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Ok, for the new flags (and cabal-install talking to Setup built with older versions of Cabal lib).

dcoutts commented on a46d772 Dec 6, 2013

Ok, for the new flags (and cabal-install talking to Setup built with older versions of Cabal lib).

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Yes, good.

dcoutts commented on c0e401b Dec 6, 2013

Yes, good.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

I presume we do pass all flags right?

dcoutts commented on fb58429 Dec 6, 2013

I presume we do pass all flags right?

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

I'll have to check, but if no, we should.

Owner

23Skidoo replied Dec 6, 2013

I'll have to check, but if no, we should.

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Right, and if we make sure that Setup configure --exact-configuration has that sanity check, then we'll find out :-)

Right, and if we make sure that Setup configure --exact-configuration has that sanity check, then we'll find out :-)

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

Yes, it looks like we do.

Owner

23Skidoo replied Dec 6, 2013

Yes, it looks like we do.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Ok.

dcoutts commented on 5583563 Dec 6, 2013

Ok.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Ahh, this is an internal dependency. Could probably give that a named local function so it's clearer.

Ahh, this is an internal dependency. Could probably give that a named local function so it's clearer.

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

Fixed.

Owner

23Skidoo replied Dec 6, 2013

Fixed.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 6, 2013

Hmm, a few issues here.

Firstly, we're not actually relaxing the constraints from the .cabal file, so the solver (in finalizePackageDescription) should fail if we give a dep on a package that's not allowed by the constraints in the .cabal file. OTOH, you say it works, but my reading is that it ought to fail, so what am I missing?

Secondly, I think for --exact-configuration we ought to have a sanity check that all the deps and flags are indeed specified.

I realise that finalizePackageDescription is rather limiting, but eventually (if I ever get any further with my parser + GenericPackageDescription rewrite) we'll have finalizePackageDescription split into two: a solver and a function that applies a solution to go from a GenericPackageDescription to a PackageDescription. With --exact-configuration we will be able to skip the solver bit and go straight to a solution, but obviously that relies on the solution being complete. So I think the sanity check is important (cabal-install could get it wrong and specify incomplete info which would be partially covered up by the finalizePackageDescription filling in the remaining info -- leading to really hard to diagnose bug reports).

dcoutts commented on d87b584 Dec 6, 2013

Hmm, a few issues here.

Firstly, we're not actually relaxing the constraints from the .cabal file, so the solver (in finalizePackageDescription) should fail if we give a dep on a package that's not allowed by the constraints in the .cabal file. OTOH, you say it works, but my reading is that it ought to fail, so what am I missing?

Secondly, I think for --exact-configuration we ought to have a sanity check that all the deps and flags are indeed specified.

I realise that finalizePackageDescription is rather limiting, but eventually (if I ever get any further with my parser + GenericPackageDescription rewrite) we'll have finalizePackageDescription split into two: a solver and a function that applies a solution to go from a GenericPackageDescription to a PackageDescription. With --exact-configuration we will be able to skip the solver bit and go straight to a solution, but obviously that relies on the solution being complete. So I think the sanity check is important (cabal-install could get it wrong and specify incomplete info which would be partially covered up by the finalizePackageDescription filling in the remaining info -- leading to really hard to diagnose bug reports).

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Owner

I changed how dependencySatisfiable works. With --exact-configuration, it simply looks up the package name in the requiredDepsMap. Note that it does not do the version range check. This has the added benefit that if the package name is not in requiredDepsMap (wasn't given via --dependency), finalizePackageDescription will fail because the dependency is not satisfiable.

So I think that in the sanity check I need to only look at whether all flags are provided.

Owner

23Skidoo replied Dec 6, 2013

I changed how dependencySatisfiable works. With --exact-configuration, it simply looks up the package name in the requiredDepsMap. Note that it does not do the version range check. This has the added benefit that if the package name is not in requiredDepsMap (wasn't given via --dependency), finalizePackageDescription will fail because the dependency is not satisfiable.

So I think that in the sanity check I need to only look at whether all flags are provided.

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 9, 2013

Ok, got it, thanks.

Ok, got it, thanks.

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 6, 2013

Member

@dcoutts I think I've now addressed all your comments.

Member

23Skidoo commented Dec 6, 2013

@dcoutts I think I've now addressed all your comments.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 7, 2013

Member

Thanks, I'll have another look.

Member

dcoutts commented Dec 7, 2013

Thanks, I'll have another look.

@bos

This comment has been minimized.

Show comment Hide comment
@bos

bos Dec 9, 2013

Contributor

I tried this last night, and it absolutely saved my bacon. Thanks, @23Skidoo!

Contributor

bos commented Dec 9, 2013

I tried this last night, and it absolutely saved my bacon. Thanks, @23Skidoo!

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 9, 2013

Thanks, clearer.

dcoutts commented on 627a37f Dec 9, 2013

Thanks, clearer.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 9, 2013

Great.

dcoutts commented on 8910285 Dec 9, 2013

Great.

@dcoutts

This comment has been minimized.

Show comment Hide comment
@dcoutts

dcoutts Dec 9, 2013

Member

@23Skidoo ok, I think we're good to go.

@bos ah, good to know. Thanks for trying it. Perhaps mention it in an update on your blog/G+? ;-)

Member

dcoutts commented Dec 9, 2013

@23Skidoo ok, I think we're good to go.

@bos ah, good to know. Thanks for trying it. Perhaps mention it in an update on your blog/G+? ;-)

23Skidoo added a commit that referenced this pull request Dec 9, 2013

Merge pull request #1604 from 23Skidoo/allow-newer
Implement the '--allow-newer' option for 'configure'/'install'

@23Skidoo 23Skidoo merged commit 9fb3927 into haskell:master Dec 9, 2013

@23Skidoo 23Skidoo deleted the 23Skidoo:allow-newer branch Dec 9, 2013

@23Skidoo

This comment has been minimized.

Show comment Hide comment
@23Skidoo

23Skidoo Dec 9, 2013

Member

@bos Glad to hear that!

Member

23Skidoo commented Dec 9, 2013

@bos Glad to hear that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment