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

Don't implement allow-newer/allow-older by modifying PackageDescription #4203

Open
ezyang opened this issue Jan 3, 2017 · 10 comments
Open

Comments

@ezyang
Copy link
Contributor

ezyang commented Jan 3, 2017

It seems cleaner to hook it straight into the dep solver, rather than try to ninja it in by package description modification. If you do it by modification, you have to worry about making sure the "correct" package description is used anywhere you want to look at deps, and sometimes you get spurious warnings because it looks like you modify build-depends which affects our source level checks, but actually we didn't.

The lack of cleanliness here is hurting me because I want BuildInfo to reflect the written syntax as closely as possible (when I make the modification for #4155), but I have to keep targetBuildDepends around as a real field if people are editing it.

Previously @kosmikus requested this in #3466.

@Ericson2314
Copy link
Collaborator

Ericson2314 commented Jan 3, 2017

This would be in the same spirit as removing cabal-install's mysterious other injected extra bounds which cause #4169 right?

@ezyang
Copy link
Contributor Author

ezyang commented Jan 3, 2017

Yep, I think so, thanks for posting the reference!

@23Skidoo
Copy link
Member

23Skidoo commented Jan 3, 2017

The problem here is that we won't be able to implement ./Setup.hs configure --alow-newer in this way, and IIRC that interface is used by Stack (it was added per @snoyberg's request).

@hvr
Copy link
Member

hvr commented Jan 3, 2017

@23Skidoo relevant: #3581

@23Skidoo
Copy link
Member

23Skidoo commented Jan 3, 2017

OK, if Stack doesn't need that interface, then we can remove it.

hvr added a commit to hvr/cabal that referenced this issue May 19, 2017
This is a preparatory refactoring needed for future work such as haskell#4203.

I've refrained from doing additional cleanups in order to keep this a
refactoring that mostly moves around blocks of code mostly
unchanged (except for whitespace), and make it easier to review.

This feature was originally implemented because its lack was complained
about by Stack/Stackage developers. However, after it got implemented it
was never really being used; what's more, it's causing us overhead for
no benefit as well as blocking us improving the implementation via the
likes of haskell#4203.

Closes haskell#3581
@Ericson2314
Copy link
Collaborator

Ericson2314 commented Feb 1, 2018

Because of #4527, this only covers other cases of modifying bounds in situ like #4169, and #4383 if it's merged, right?

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

#4527 only removed the --allow-{newer,older} command-line options from setup scripts, the main issue this ticket is about is still there.

@Ericson2314
Copy link
Collaborator

@23Skidoo Ah the code in Cabal is still there and not dead via something else?

@23Skidoo
Copy link
Member

23Skidoo commented Feb 1, 2018

Yes, it's spread across lib:Cabal and cabal-install. See D.C.Dependency.removeBounds, which uses D.PD.transformAllBuildDepends.

@Ericson2314
Copy link
Collaborator

Thanks!

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

No branches or pull requests

5 participants