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

Allow lens 5.2 #1607

Merged
merged 1 commit into from Oct 2, 2022
Merged

Conversation

l-epple
Copy link
Contributor

@l-epple l-epple commented Sep 2, 2022

No code changes were required, just needed to bump the upper bounds.
servant-swagger already allows 5.2. In a normal situation you won't get
lens 5.2 yet, because swagger2 currently disallows lens 5.2:
GetShopTV/swagger2#242
Additionally, servant-multipart needs its constraint lifted as well
which I'll PR to the respective repository.

Tested using

cabal v2-build all --allow-newer=lens --constraint='lens >= 5.2'

No code changes were required, just needed to bump the upper bounds.
servant-swagger already allows 5.2. In a normal situation you won't get
lens 5.2 yet, because swagger2 currently disallows lens 5.2:
GetShopTV/swagger2#242
Additionally, servant-multipart needs its constraint lifted as well
which I'll PR to the respective repository.

Tested using

    cabal v2-build all --allow-newer=lens --constraint='lens >= 5.2'
l-epple added a commit to possehl-analytics/servant-multipart that referenced this pull request Sep 2, 2022
@ysangkok
Copy link
Contributor

ysangkok commented Sep 6, 2022

I'd suggest using allow-newer only for specific packages, like e.g. --allow-newer=swagger2:lens. That way, people can check if the allow-newer was already resolved upstream, when they review this.

See also the cabal.project in #1592: https://github.com/haskell-servant/servant/pull/1592/files#diff-b8ed06757045fac949c89f2139a862498ad2b6d1f82c61a31e7c91d6cf0eaa70

@l-epple
Copy link
Contributor Author

l-epple commented Sep 9, 2022

Thanks, I wasn't aware you can restrict the allow-newer flag further. I tested it like this:

cabal v2-build all --constraint='lens >= 5.2' --allow-newer='swagger2:lens' --allow-newer='servant-js:base' --allow-newer='servant-js:lens'  --allow-newer='servant-multipart:lens

So seems like we also need to update servant-js which lives, I think, in a separate repository, but this shouldn't block this PR.

Do you want me to include an updated cabal.project in this PR as well?

@ysangkok
Copy link
Contributor

@l-epple My personal preference is not to commit allow-newer lines because they cause unnecessary churn all over the ecosystem. They become unnecessary automatically, but we have no tooling to detect when they are unnecessary. I'd prefer if we'd just fix the dependencies first instead, which is totally doable given that trustees can bump bounds if maintainers become unresponsive.

But if code changes need do be done, I think it makes sense to parallelize work, since that often requires more review, and trustees can't do code changes. In that case, my preference is to put the allow-newers in the PR discussion, as you have done (even though I recognize that this PR has no code changes).

I probably haven't been consistently waiting with sending PRs until no allow-newers are necessary. So all of this is up for discussion, these are just my two cents. I'd like to hear your thoughts on this.

The Servant repo seems to be an outlier to me since it has so many projects inside, and therefore a huge dependency tree. This is a shame, because even small changes quickly become blocked on some dependency. For example, take that GHC-9.4 PR I linked. Many parts of the project would already be compatible if bounds were bumped, and their changes could be released today. But because other parts of the code base do require doctest, the whole PR is blocked on this.

It is even possible to not build, or ignore doctest failures for specific GHC versions. But this also causes churn, so I'd consider it just as bad as cabal.project.

Hopefully the new code-generators feature in Cabal 3.8 will inspire someone to make a doctest library with a lower maintenance burden.

@ysangkok ysangkok mentioned this pull request Oct 2, 2022
@tchoutri tchoutri merged commit e4650de into haskell-servant:master Oct 2, 2022
maksbotan pushed a commit to haskell-servant/servant-multipart that referenced this pull request Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants