Add check for upper bound on any dependency in cabal check #8339

jappeace · 2022-08-03T19:37:47Z

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:

Patches conform to the coding conventions.
Any changes that could be relevant to users have been recorded in the changelog.
The documentation has been updated, if necessary.

Please also shortly describe how you tested your change. Bonus points for added tests!

fixes haskell#8291 presumably this will make it nag at anyone for forgetting to add upper bounds to their packages. add changelog (presumably)

ffaf1 · 2022-08-03T20:15:25Z

Cabal/src/Distribution/PackageDescription/Check.hs

@@ -669,6 +670,9 @@ ppExplanation (UnknownArch unknownArches) =
    "Unknown architecture name " ++ commaSep (map quote unknownArches)
 ppExplanation (UnknownCompiler unknownImpls) =
    "Unknown compiler name " ++ commaSep (map quote unknownImpls)
+ppExplanation (MissingUpperBounds pname) =
+  "'" ++ unPackageName pname ++ "' misses upper bounds, add them"


You can probably the convenience quote instead of "'" ++ xyz "'"

Also why not indenting four spaces, since that is what is been used in ppExplanation.

Mikolaj · 2022-08-03T20:22:38Z

@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.

ffaf1 · 2022-08-03T20:25:39Z

Cabal/src/Distribution/PackageDescription/Check.hs

@@ -669,6 +670,9 @@ ppExplanation (UnknownArch unknownArches) =
    "Unknown architecture name " ++ commaSep (map quote unknownArches)
 ppExplanation (UnknownCompiler unknownImpls) =
    "Unknown compiler name " ++ commaSep (map quote unknownImpls)
+ppExplanation (MissingUpperBounds pname) =
+  "'" ++ unPackageName pname ++ "' misses upper bounds, add them"


A list of dependencies missing upper bounds would e great (plus, the specific target).

what's a target?

ffaf1 · 2022-08-03T20:28:02Z

Cabal/src/Distribution/PackageDescription/Check.hs

@@ -2398,6 +2391,18 @@ checkGlobFiles verbosity pkg root =
    getWarning field glob (GlobMissingDirectory dir) =
      [ PackageDistSuspiciousWarn (GlobNoDir field glob dir) ]

+toDependencyVersionsMap :: (PackageDescription -> [Dependency]) -> GenericPackageDescription -> Map PackageName VersionRange


Why is this function at this line (i.e.: far away from where it is used, not in the utils part of the file)?

I moved this to the utils section

ffaf1 · 2022-08-03T20:31:16Z

Good stab @jappeace. I have added some comments, hopefully you don't mind.
I can help with testing/formatting if necessary; happy hacking!

fgaz · 2022-08-03T21:21:50Z

Cabal-tests/tests/ParserTests/regressions/public-multilib-1.cabal

@@ -7,7 +7,7 @@ license:       MIT

 library
  default-language: Haskell2010
-  build-depends:    base ^>=4.14, internal
+  build-depends:    base ^>=4.14, internal ^=0


I think the requirement should be lifted for libraries belonging to the same package

fgaz · 2022-08-03T21:22:40Z

Cabal/src/Distribution/PackageDescription/Check.hs

@@ -669,6 +670,9 @@ ppExplanation (UnknownArch unknownArches) =
    "Unknown architecture name " ++ commaSep (map quote unknownArches)
 ppExplanation (UnknownCompiler unknownImpls) =
    "Unknown compiler name " ++ commaSep (map quote unknownImpls)
+ppExplanation (MissingUpperBounds pname) =
+  "'" ++ unPackageName pname ++ "' misses upper bounds, add them"


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

ulysses4ever · 2022-08-10T14:07:21Z

@jappeace just FYI: you're double-lucky now, as it's the second time you got into borked CI situation (tracked as #8356).

jappeace · 2022-08-10T15:16:12Z

Cabal-tests/tests/ParserTests/regressions/public-multilib-2.cabal

@@ -9,6 +9,6 @@ library
  default-language: Haskell2010
  build-depends:
    , base     ^>=4.14
-    , somelib:internal
+    , somelib:internal ^>=1.0


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).

I think somelib is a dep, internal is its component.

... probably not the best approach

jappeace · 2022-08-10T19:03:23Z

@ulysses4ever hitting it now, I guess this technically passes, thanks for letting me know.

Mikolaj · 2022-08-11T10:19:38Z

Let me rebase to see if the recent CI workaround unblocks your testing.

Mikolaj · 2022-08-11T10:19:46Z

@mergify rebase

mergify · 2022-08-11T10:19:55Z

rebase

❌ Pull request can't be updated with latest base branch changes

Mergify needs the author permission to update the base branch of the pull request.
@SupercedeTech needs to authorize modification on its head branch.
err-code: D81BA

Mikolaj · 2022-08-11T10:48:12Z

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?

jappeace · 2022-08-11T10:57:35Z

I've done nothing but the defaults 😅
How does one unlock branches?

Mikolaj · 2022-08-11T11:04:12Z

"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.

jappeace · 2022-08-11T11:24:46Z

I pressed a big "update" button, probably wrong. I'll just re-open this from my personal account, that should make it work

Mikolaj · 2022-08-11T11:30:19Z

Sorry for the inconvenience. If that's too much trouble, we can work around, abusing our review process.

jappeace · 2022-08-11T11:36:36Z

recreated in #8361

jappeace changed the title ~~Add check for upper bound on any package~~ Add check for upper bound on any dependency in cabal check Aug 3, 2022

Add check for upper bound on any package

fb30717

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 August 3, 2022 19:39

wait what?

ea851dc

ffaf1 reviewed Aug 3, 2022

View reviewed changes

fgaz reviewed Aug 3, 2022

View reviewed changes

jappeace added 6 commits August 4, 2022 20:27

move toDependencyVersionsMap to utils section

755470f

add nicer error message

b8a9ad3

simplify checking logic, add more comments

969743a

only emit missing upper bounds if bigger then one

4925aa7

don't add bound to internal libraries

d267e42

filter out self from the dependency map

032172a

jappeace added 2 commits August 10, 2022 16:59

I think this is an external library so it needs an upper bound now?

a4e67ee

add test for multilib

d19f1ca

jappeace commented Aug 10, 2022

View reviewed changes

fix test suite by ignoring the warning

d408e91

... probably not the best approach

Merge branch 'master' into add-upper-bound-check-to-all

08bf672

jappeace mentioned this pull request Aug 11, 2022

Add upper bound check to all #8361

Merged

3 tasks

jappeace closed this Aug 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for upper bound on any dependency in cabal check #8339

Add check for upper bound on any dependency in cabal check #8339

jappeace commented Aug 3, 2022 •

edited

Loading

ffaf1 Aug 3, 2022

ffaf1 Aug 3, 2022

Mikolaj commented Aug 3, 2022

ffaf1 Aug 3, 2022

jappeace Aug 4, 2022

ffaf1 Aug 3, 2022

jappeace Aug 4, 2022

ffaf1 commented Aug 3, 2022

fgaz Aug 3, 2022

fgaz Aug 3, 2022

ulysses4ever commented Aug 10, 2022

jappeace Aug 10, 2022

Mikolaj Aug 10, 2022

jappeace commented Aug 10, 2022

Mikolaj commented Aug 11, 2022

Mikolaj commented Aug 11, 2022

mergify bot commented Aug 11, 2022

Mikolaj commented Aug 11, 2022

jappeace commented Aug 11, 2022

Mikolaj commented Aug 11, 2022

jappeace commented Aug 11, 2022 •

edited

Loading

Mikolaj commented Aug 11, 2022 •

edited

Loading

jappeace commented Aug 11, 2022

Add check for upper bound on any dependency in cabal check #8339

Add check for upper bound on any dependency in cabal check #8339

Conversation

jappeace commented Aug 3, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mikolaj commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffaf1 commented Aug 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses4ever commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jappeace commented Aug 10, 2022

Mikolaj commented Aug 11, 2022

Mikolaj commented Aug 11, 2022

mergify bot commented Aug 11, 2022

❌ Pull request can't be updated with latest base branch changes

Mikolaj commented Aug 11, 2022

jappeace commented Aug 11, 2022

Mikolaj commented Aug 11, 2022

jappeace commented Aug 11, 2022 • edited Loading

Mikolaj commented Aug 11, 2022 • edited Loading

jappeace commented Aug 11, 2022

jappeace commented Aug 3, 2022 •

edited

Loading

jappeace commented Aug 11, 2022 •

edited

Loading

Mikolaj commented Aug 11, 2022 •

edited

Loading