Add check for upper bound on any dependency in cabal check#8339
Add check for upper bound on any dependency in cabal check#8339jappeace wants to merge 12 commits intohaskell:masterfrom
Conversation
jappeace
changed the title
fixes haskell#8291 presumably this will make it nag at anyone for forgetting to add upper bounds to their packages. add changelog (presumably)
jappeace
force-pushed
the
add-upper-bound-check-to-all
branch
from
84d54a0 to
fb30717
Compare
| ppExplanation (UnknownCompiler unknownImpls) = | ||
| "Unknown compiler name " ++ commaSep (map quote unknownImpls) | ||
| ppExplanation (MissingUpperBounds pname) = | ||
| "'" ++ unPackageName pname ++ "' misses upper bounds, add them" |
There was a problem hiding this comment.
You can probably the convenience quote instead of "'" ++ xyz "'"
There was a problem hiding this comment.
Also why not indenting four spaces, since that is what is been used in ppExplanation.
|
@jappeace: you are lucky and your patch tripped an existing test. Perhaps just fix the test and so cover negative side of the testing story and then copy-paste, add the upper bound, simplify and the positive side (no warning) is covered as well. Thank you for the tiny changelog file. That's very helpful for the poor chap making releases. |
| ppExplanation (UnknownCompiler unknownImpls) = | ||
| "Unknown compiler name " ++ commaSep (map quote unknownImpls) | ||
| ppExplanation (MissingUpperBounds pname) = | ||
| "'" ++ unPackageName pname ++ "' misses upper bounds, add them" |
There was a problem hiding this comment.
A list of dependencies missing upper bounds would e great (plus, the specific target).
| getWarning field glob (GlobMissingDirectory dir) = | ||
| [ PackageDistSuspiciousWarn (GlobNoDir field glob dir) ] | ||
|
|
||
| toDependencyVersionsMap :: (PackageDescription -> [Dependency]) -> GenericPackageDescription -> Map PackageName VersionRange |
There was a problem hiding this comment.
Why is this function at this line (i.e.: far away from where it is used, not in the utils part of the file)?
There was a problem hiding this comment.
I moved this to the utils section
|
Good stab @jappeace. I have added some comments, hopefully you don't mind. |
| library | ||
| default-language: Haskell2010 | ||
| build-depends: base ^>=4.14, internal | ||
| build-depends: base ^>=4.14, internal ^=0 |
There was a problem hiding this comment.
I think the requirement should be lifted for libraries belonging to the same package
| ppExplanation (UnknownCompiler unknownImpls) = | ||
| "Unknown compiler name " ++ commaSep (map quote unknownImpls) | ||
| ppExplanation (MissingUpperBounds pname) = | ||
| "'" ++ unPackageName pname ++ "' misses upper bounds, add them" |
There was a problem hiding this comment.
Maybe a gentler "consider adding them with...", since for now it's just a PackageDistSuspiciousWarn. Also I wonder if we should add (a link to) a longer explanation of the problem
| build-depends: | ||
| , base ^>=4.14 | ||
| , somelib:internal | ||
| , somelib:internal ^>=1.0 |
There was a problem hiding this comment.
I'm uncertain what this syntax somelib:internal means, I assumed somelib is a dependency.
There appears no docs on this syntax (note that this is the other multilib failing now, not mulitlib 1, which I fixed).
There was a problem hiding this comment.
I think somelib is a dep, internal is its component.
... probably not the best approach
|
@ulysses4ever hitting it now, I guess this technically passes, thanks for letting me know. |
|
Let me rebase to see if the recent CI workaround unblocks your testing. |
|
@mergify rebase |
❌ Pull request can't be updated with latest base branch changesDetailsMergify needs the author permission to update the base branch of the pull request. |
|
Uhoh, @jappeace, it seems your branch is protected from meddling by the sketchy cabal admins, which is understandable, but also implies that Mergify won't be able to merge your PR. Would it be possible to unlock the branch? |
|
I've done nothing but the defaults 😅 |
|
"Allow edits and access to secrets by maintainers" at the bottom right, if it exists? Probably your or your org's (SupercedeTech?) defaults are more strict than normally. |
|
I pressed a big "update" button, probably wrong. I'll just re-open this from my personal account, that should make it work |
|
Sorry for the inconvenience. If that's too much trouble, we can work around, abusing our review process. |
|
recreated in #8361 |
5 participants
fixes #8291
presumably this will make it nag at anyone for forgetting
to add upper bounds to their packages.
I tested this by running cabal check on yesod-auth-simple (which has no upper bounds)
then added said upper bounds with cabal gen-bounds, and now it works.
I'd love to add a small minimal test suite if someone explains how to do it 😅
Please include the following checklist in your PR:
Please also shortly describe how you tested your change. Bonus points for added tests!