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

Additional version bound checks #10554

Merged
merged 1 commit into from
Jan 5, 2025

Conversation

philderbeast
Copy link
Collaborator

@philderbeast philderbeast commented Nov 18, 2024

Fixes #9806. Checks that lower bounds are inclusive, upper bounds are exclusive and don't have trailing zeros.

Copy link
Collaborator

@ulysses4ever ulysses4ever left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 18, 2024

And of course, if something is not clear in the checks scaffold, ask!

@philderbeast
Copy link
Collaborator Author

philderbeast commented Nov 18, 2024

Converted to draft as I think there's more work to do:

$ cabal init "version-checks" --non-interactive
Warning: this is a debug build of cabal-install with assertions enabled.
[Info] Guessing dependencies...
[Info] Using cabal specification: 3.0
[Warn] unknown license type, you must put a copy in LICENSE yourself.
[Info] Creating fresh file CHANGELOG.md...
[Info] Creating fresh directory ./app...
[Info] Creating fresh file app/Main.hs...
[Info] Creating fresh file version-checks.cabal...
[Warn] No synopsis given. You should edit the .cabal file and add one.
[Info] You may want to edit the .cabal file and add a Description field.

$ cd version-checks/

$ grep -R -E 'base' *.cabal
    build-depends:    base ^>=4.20.0.0

$ cabal check
Warning: this is a debug build of cabal-install with assertions enabled.
These warnings will likely cause trouble when distributing the package:
Warning: [no-category] No 'category' field.
These warnings may cause trouble when distributing the package:
Warning: [less-than-equals-upper-bounds] On executable 'version-checks', these
packages have less than or equals (<=) upper bounds:
- base
Please use less than (<) for upper bounds. There is more information at
https://pvp.haskell.org/
Warning: [trailing-zero-upper-bounds] On executable 'version-checks', these
packages have upper bounds with trailing zeros:
- base
Please avoid trailing zeros for upper bounds. There is more information at
https://pvp.haskell.org/
Warning: [greater-than-lower-bounds] On executable 'version-checks', these
packages have greater than (>) lower bounds:
- base
Please use greater than or equals (>=) for lower bounds. There is more
information at https://pvp.haskell.org/
The following errors will cause portability problems on other environments:
Error: [no-syn-desc] No 'synopsis' or 'description' field.
Error: [license-none] The 'license' field is missing or is NONE.
Error: Hackage would reject this package.

@philderbeast philderbeast force-pushed the fix/check-version-bounds branch 3 times, most recently from 8ca45cf to b2e539b Compare November 19, 2024 23:14
@philderbeast philderbeast marked this pull request as ready for review November 20, 2024 19:43
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch from edf1712 to 50313b1 Compare November 21, 2024 15:01
@philderbeast philderbeast requested a review from ffaf1 November 21, 2024 15:07
@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 21, 2024

Thanks, most likely I will review this this weekend.

@philderbeast philderbeast force-pushed the fix/check-version-bounds branch from 50313b1 to d40acfd Compare November 23, 2024 14:32
Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work.
I have some things I don't agree with and I was overt in my thoughts (apologies if I missed some of your earlier notes).

I am sure we can work together to make this in master speedily~

Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated Show resolved Hide resolved
changelog.d/pr-10554 Show resolved Hide resolved
changelog.d/pr-10554 Outdated Show resolved Hide resolved
Cabal-syntax/src/Distribution/Types/VersionRange.hs Outdated Show resolved Hide resolved
Cabal-syntax/src/Distribution/Version.hs Outdated Show resolved Hide resolved
Cabal/src/Distribution/PackageDescription/Check.hs Outdated Show resolved Hide resolved
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch 4 times, most recently from 7be19e7 to 5bef5e7 Compare November 25, 2024 21:37
@ffaf1
Copy link
Collaborator

ffaf1 commented Nov 29, 2024

@philderbeast is this ready for a re-review?

(no fret if it is not)

@philderbeast philderbeast force-pushed the fix/check-version-bounds branch from 5bef5e7 to 7e74f9c Compare November 29, 2024 20:33
@philderbeast
Copy link
Collaborator Author

@ffaf1 I've rebased and this is ready for review.

@philderbeast philderbeast requested a review from ffaf1 November 29, 2024 20:33
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch 3 times, most recently from d39851e to 498ca97 Compare December 7, 2024 01:15
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch from 498ca97 to a7ba282 Compare December 9, 2024 18:04
@ffaf1
Copy link
Collaborator

ffaf1 commented Dec 10, 2024

I have added a few more comments @philderbeast.

If you need help or are ready for another review, just ping me and I will chime in!

@philderbeast philderbeast force-pushed the fix/check-version-bounds branch 2 times, most recently from d767d43 to 0d787e3 Compare December 14, 2024 18:48
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch 2 times, most recently from 5dcdb5b to d59f4fc Compare December 29, 2024 17:14
@ffaf1
Copy link
Collaborator

ffaf1 commented Jan 2, 2025

Ping me when this is ready, almost everything I see is very good.

@philderbeast
Copy link
Collaborator Author

@ffaf1 I've moved all of the dependency version range checks to cabal-testsuite/PackageTests/Check/DepVersionRange.

$ cd cabal-testsuite/PackageTests/Check/DepVersionRange

$ tree -P '*.test.hs'
.
├── BaseDep
│   └── cabal.test.hs
├── CustomSetupBaseDep
│   └── cabal.test.hs
├── InternalLibDep
│   └── cabal.test.hs
├── UnboundedInternalDep
│   └── cabal.test.hs
└── VersionConstraintOperators
    └── cabal.test.hs

As for missing bounds checks, I don't think we have any duplication:

  • BaseDep Error: [missing-bounds-important] is a check on upper bounds of base.
  • CustomSetupBaseDep Error: [missing-bounds-setup] is a check that a custom setup has upper bounds on base.
  • InternalLibDep Warning: [missing-upper-bounds] is a check of upper bounds of the dependencies an internal library (but not on the dependencies of the main package library).
  • UnboundedInternalDep Warning: [missing-upper-bounds] is a check of internal dependencies, of an executable component depending on the library component within the same package.
  • VersionConstraintOperators Warning: [missing-upper-bounds] checks version operators and missing upper bounds of the main (unnamed1) package library.
$ grep -R -E '\[missing-upper-bounds\] On library' */**/*.out
InternalLibDep/cabal.out:Warning: [missing-upper-bounds] On library 'int-lib', these packages miss upper bounds:
VersionConstraintOperators/cabal.out:Warning: [missing-upper-bounds] On library, these packages miss upper bounds:

Footnotes

  1. In this case shouldn't we be reporting "On library 'package-name'" instead of "On library"?

Copy link
Collaborator

@ffaf1 ffaf1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the excellent work!

@philderbeast
Copy link
Collaborator Author

Label merge+no rebase is necessary when the pull request is from an organisation.

@mergify mergify bot added the ready and waiting Mergify is waiting out the cooldown period label Jan 2, 2025
@philderbeast
Copy link
Collaborator Author

Thanks for the thorough reviews @ffaf1.

- Check for LEQ upper bounds
- Check for GT lower bounds
- Check for trailing zero upper bounds
- Add missing gtLowerBound to checks
- Handle ^>= versions with its IntersectVersionRangesF
- Set baseline for cabal init generated bounds
- Use recursive functions for checking bounds
- Handle union version ranges
- Update test expectations with --accept in other tests
- Use inclusive lower bound for issue-8646.cabal
- Satisfy the parsimonious test for messages
- Allow exceptions to 25 char limit explain ids
- Rename Is* to Has* to match previous predicates
- Move predicates to VersionRange module
- Add changelog
- Remove unit-test guards that aren't needed
- Shorten check IDs
- Add warnings to cabal check section of user guide
- Terminate bulleted list with full stop
- Remove links to pvp.haskell.org
- Note version constraint guidelines and mistakes
- Add listSep
- Reuse queryVersionRange
- Bundle pattern synonyms with VersionRangeF
- Add doctest docs for version range predicates
- Used named chunk for predicate examples
- Add predicate subsections for types of bounds
- Change lte- to le- prefix
- Satisfy fourmolu
- Flip sense of LE and GT haddocks
- Drop Has prefix on patterns, use LE not LEQ
- avoid name clash with has*Bound (VersionRange -> Bool) predicates
- use TZ not TrailingZero, a two-letter prefix like the other two
- Test expectations with shorter check messages
- Remove unused LANGUAGE pragmas
- Satisfy hlint
- Promote to haddocks, move NOTE about dashes
- Rerun --accept test to generate VersionBound/cabal.out
- rewrite was not triggered by only a trailing whitespace differerence
- Fixup version range of ImpossibleVersionRangeLib
- Fixup version range of NonConfCheck/PackageVersionsInternal
- Fixup version range of NonConfCheck/PackageVersionsInternalSimple
- Fixup version range of NonConfCheck/PackageVersionsLibInt
- Fixup version range of NonConfCheck/PackageVersionsStraddle
- Fixup version range of NonConfCheck/SetupBounds
- Move to DepVersionRange/VersionConstraintOperators
- Move to DepVersionRange/UnboundedInternalDep
- Move to DepVersionRange/InternalLibDep
- Move to DepVersionRange/BaseDep
- Move to DepVersionRange/CustomSetupBaseDep
@philderbeast philderbeast force-pushed the fix/check-version-bounds branch from ee33aa2 to d46f325 Compare January 3, 2025 10:28
@mergify mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 5, 2025
@mergify mergify bot merged commit e98bc7f into haskell:master Jan 5, 2025
55 checks passed
@Kleidukos
Copy link
Member

@Mergifyio backport 3.14

Copy link
Contributor

mergify bot commented Jan 14, 2025

backport 3.14

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days merge+no rebase ready and waiting Mergify is waiting out the cooldown period
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cabal check: Warn about "bad" bounds
5 participants