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

Implement --allow-older #3466

Merged
merged 1 commit into from
Jul 23, 2016
Merged

Implement --allow-older #3466

merged 1 commit into from
Jul 23, 2016

Conversation

hvr
Copy link
Member

@hvr hvr commented May 29, 2016

This provides the dual flag to --allow-newer for symmetry.

The need for relaxing lower bounds was suggested by @mgsloan here.

@hvr
Copy link
Member Author

hvr commented May 29, 2016

Unfortunately, AllowNewer was moved into the Cabal library thereby making it part of the public API :-(

So I didn't rename the AllowNewer type to avoid API breakage, nor did I introduce a type AllowOlder as it would have resulted in code duplication for little gain.

TODO:

  • documentation
  • changelog
  • testsuite

@23Skidoo
Copy link
Member

Since this is targeted at master, why is it important to avoid API breakage?

Also seems to break Travis.

@hvr
Copy link
Member Author

hvr commented May 29, 2016

@23Skidoo If it's ok to break the API in Cabal 1.26 I'll happily rename the type to something like AllowScope or RelaxScope (any better ideas?)

and Travis fails because I didn't adapt the testsuite yet...

@23Skidoo
Copy link
Member

I think it's fine to break the API in this case. It's unlikely that this will break any setup scripts.

@hvr
Copy link
Member Author

hvr commented May 30, 2016

@23Skidoo do you have any preference for how to rename the current AllowNewer type? Also I'd do this in a separate commit, maybe even in a spearate PR, as this will likely touch a lot of code.

For reference, here's the affected types that would need renaming to become less specific to Newer:

-- | Policy for relaxing upper bounds in dependencies. For example, given
-- 'build-depends: array >= 0.3 && < 0.5', are we allowed to relax the upper
-- bound and choose a version of 'array' that is greater or equal to 0.5? By
-- default the upper bounds are always strictly honored.
data AllowNewer =

  -- | Default: honor the upper bounds in all dependencies, never choose
  -- versions newer than allowed.
  AllowNewerNone

  -- | Ignore upper bounds in dependencies on the given packages.
  | AllowNewerSome [AllowNewerDep]

  -- | Ignore upper bounds in dependencies on all packages.
  | AllowNewerAll
  deriving (Eq, Read, Show, Generic)

-- | Dependencies can be relaxed either for all packages in the install plan, or
-- only for some packages.
data AllowNewerDep = AllowNewerDep PackageName
                   | AllowNewerDepScoped PackageName PackageName
                   deriving (Eq, Read, Show, Generic)

@23Skidoo
Copy link
Member

23Skidoo commented May 30, 2016

@hvr I think that something like the following should work:

data RelaxDeps = RelaxDepsNone | RelaxDepsSome [RelaxedDep] | RelaxDepsAll

data RelaxedDep = RelaxedDep PackageName
                | RelaxedDepScoped PackageName PackageName

newtype AllowNewer = AllowNewer RelaxDeps
newtype AllowOlder = AllowOlder RelaxDeps

@dcoutts
Copy link
Contributor

dcoutts commented May 31, 2016

Seems reasonable.

@hvr
Copy link
Member Author

hvr commented Jun 4, 2016

@23Skidoo @dcoutts ok, so you want this typesafe via newtypes to reduce the risk of mixing up codepaths related to newer/older? :-)

hvr added a commit to hvr/cabal that referenced this pull request Jun 4, 2016
This also adds a not yet used `AllowOlder` newtype

This is a preparatory refactoring propsed in haskell#3466 for supporting `--allow-older`
hvr added a commit to hvr/cabal that referenced this pull request Jun 4, 2016
This also adds a not yet used `AllowOlder` newtype

This is a preparatory refactoring propsed in haskell#3466 for supporting `--allow-older`
hvr added a commit to hvr/cabal that referenced this pull request Jun 5, 2016
This also adds a not yet used `AllowOlder` newtype

This is a preparatory refactoring propsed in haskell#3466 for supporting `--allow-older`
hvr added a commit to hvr/cabal that referenced this pull request Jun 5, 2016
This also adds a not yet used `AllowOlder` newtype

This is a preparatory refactoring propsed in haskell#3466 for supporting `--allow-older`
@hvr hvr force-pushed the pr/allow-older branch from 26971b2 to b82e5fb Compare June 5, 2016 22:57
@hvr
Copy link
Member Author

hvr commented Jun 6, 2016

@23Skidoo @dcoutts I've rebased this on top of #3476 and the testsuite still compiles... yay!

@hvr hvr modified the milestones: Cabal 1.26, cabal-install 1.26 Jun 6, 2016
@kosmikus
Copy link
Contributor

kosmikus commented Jul 5, 2016

This PR is marked "solver", but the way it's implemented, it does not seem to touch solver code at all.

In principle, I'd prefer somewhat if both --allow-newer and --allow-older would actually be implemented by tweaking the solver rather than by tweaking the index, but that would be a major change.

From what I can see, this PR looks useful and ok, but it touches a lot of modules I don't know very much about.

ezyang pushed a commit that referenced this pull request Jul 11, 2016
This also adds a not yet used `AllowOlder` newtype

This is a preparatory refactoring propsed in #3466 for supporting `--allow-older`
@hvr hvr force-pushed the pr/allow-older branch from b82e5fb to 7201636 Compare July 11, 2016 12:11
@hvr
Copy link
Member Author

hvr commented Jul 11, 2016

rebased against latest master

@23Skidoo
Copy link
Member

@kosmikus

In principle, I'd prefer somewhat if both --allow-newer and --allow-older would actually be implemented by tweaking the solver rather than by tweaking the index, but that would be a major change.

One problem with that is that ./Setup.hs configure --allow-newer won't be able to utilise the solver-based code path, so we'll have to maintain the current one anyway.

@23Skidoo
Copy link
Member

LGTM, I think that this can be merged once the TODO items are implemented.

@hvr
Copy link
Member Author

hvr commented Jul 22, 2016

rebased one last time & added docs/changelog/tests... once travis passes I'm gonna merge this to get this PR out of my sight... :-)

@hvr hvr force-pushed the pr/allow-older branch from 8004227 to c5b2b00 Compare July 22, 2016 19:38
hvr added a commit to hvr/cabal that referenced this pull request Jul 22, 2016
This implements the flag `--allow-older` which is the analogous to
`--allow-newer` acting on lower bounds.
This implements the flag `--allow-older` which is the analogous to
`--allow-newer` acting on lower bounds.
@hvr hvr force-pushed the pr/allow-older branch from c5b2b00 to dea1a81 Compare July 22, 2016 19:39
@hvr hvr merged commit 5944c3e into haskell:master Jul 23, 2016
@hvr hvr deleted the pr/allow-older branch July 23, 2016 13:35
@ezyang ezyang modified the milestones: cabal-install 2.0, 2.0 (planned for next feature release) Sep 6, 2016
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.

5 participants