Skip to content

Reimplement cabal check#8427

Merged
mergify[bot] merged 19 commits into
haskell:masterfrom
ffaf1:soc-pull
Nov 13, 2023
Merged

Reimplement cabal check#8427
mergify[bot] merged 19 commits into
haskell:masterfrom
ffaf1:soc-pull

Conversation

@ffaf1
Copy link
Copy Markdown
Collaborator

@ffaf1 ffaf1 commented Aug 24, 2022

Reimplementing cabal check, closes #8211, #652, #2065.

For a detailed description of the problem at hand, the proposed solution, benefits and future work check the explanatory article.

Here I will only highlight bits to ease reviews. In brief:

  • cabal check goes from “make a soup with flattenPackageDescription, check it” to “start from GenericPackageDescription and walk down, perform the appropriate checks near the relevant datatype”.
  • Same goes for conditional blocks: the various ifs inside a .cabal are merged in a single ball of mud no more. Rather we visit the CondTree in a principled manner, building the target from slices and then validating it in its appropriate context.
  • Checks happen inside CheckM m, a wrapped RSTW transformer where m abstracts over some IO/.tar archive/VCS filesystem.
  • the only API changes are one function removal (function which is not used nor in this repo, nor in hackage-server, nor in any haskell repo, nor in stack) and a signature change for checkPackage, eliminating a useless parameter. I tackled the rewrite with the idea to minimise disruption.

To the reviewer: a good way of approaching this is to to avoid the two “large diffs” files at first. Check first how other modules were impacted and the two preliminary patches. Then you can dive in PackageDescription/Check.hs:

-- ☞ N.B.
--
-- Part of the tools/scaffold used to perform check is found in
-- Distribution.PackageDescription.Check.Prim. Summary of that module (for
-- how we use it here):
-- 1. we work inside a 'Check m a' monad (where `m` is an abstraction to
--    run non-pure checks);
-- 2. 'checkP', 'checkPre' functions perform checks (respectively pure and
--    non-pure);
-- 3. 'PackageCheck' and 'CheckExplanation' are types for warning severity
--    and description.


-- ------------------------------------------------------------
-- * Checking interface
-- ------------------------------------------------------------

-- | 'checkPackagePrim' is the most general way to invoke package checks.
-- We pass to it two interfaces (one to check contents of packages, the
-- other to inspect working tree for orphan files) and before that a
-- Boolean to indicate whether we want pure checks or not. Based on these
-- parameters, some checks will be performed, some omitted.
-- Generality over @m@ means we could do non pure checks in monads other
-- than IO (e.g. a virtual filesystem, like a zip file, a VCS filesystem,
-- etc).
checkPackagePrim :: Monad m => Bool -> Maybe (CheckPackageContentOps m) ->
                    Maybe (CheckPreDistributionOps m) ->
                    GenericPackageDescription -> m [PackageCheck]
⁝

and you should find your way.

Testsuite added in #8248 and #8250 passes. As a proof of what can be done after the rewrite, this PR also:

If not merged by the time this is approved, #8441 and #8361 might need to be updated.

This patch is big, the logic of checks sometimes tricky, I expect breakage. I took care to minimise API change, this should make bugs pop up faster and allow me to fix them faster. Once everything is stable, features can be accomodated appropriately.

  • 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.
  • QA notes: calling cabal check in your project folder should warn you about common mistakes, exit with 1 on more serious ones.

@ffaf1 ffaf1 force-pushed the soc-pull branch 6 times, most recently from 02db5c5 to 39b45ff Compare September 1, 2022 13:43
@ffaf1 ffaf1 force-pushed the soc-pull branch 3 times, most recently from 28948e9 to e44f74f Compare September 10, 2022 14:10
@ffaf1 ffaf1 changed the title [WIP] Reimplement cabal check Reimplement cabal check Sep 19, 2022
@ffaf1 ffaf1 force-pushed the soc-pull branch 2 times, most recently from 54e4c30 to 70c93d7 Compare September 22, 2022 08:23
ffaf1 added a commit to ffaf1/cabal that referenced this pull request Sep 22, 2022
ffaf1 added a commit to ffaf1/cabal that referenced this pull request Sep 22, 2022
ffaf1 added a commit to ffaf1/cabal that referenced this pull request Sep 23, 2022
ffaf1 added a commit to ffaf1/cabal that referenced this pull request Sep 23, 2022
@ffaf1 ffaf1 marked this pull request as ready for review September 23, 2022 17:07
Comment thread Cabal-syntax/src/Distribution/Types/Benchmark.hs Outdated
Comment thread Cabal-tests/tests/CheckTests.hs
Comment thread Cabal-tests/tests/ParserTests/regressions/bad-glob-syntax.check
Comment thread Cabal-tests/tests/ParserTests/regressions/denormalised-paths.check
Comment thread cabal-testsuite/PackageTests/Check/ConfiguredPackage/Sanity/NoDupNames/cabal.out Outdated
@jappeace
Copy link
Copy Markdown
Collaborator

no need to wait for me, I'm good at rebasing 😉

Copy link
Copy Markdown
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

Thank you very much, Francesco! That's the big one!

Comment thread Cabal/src/Distribution/PackageDescription/Check/Conditional.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
Comment thread Cabal/src/Distribution/PackageDescription/Check/Warning.hs Outdated
ffaf1 and others added 19 commits November 11, 2023 17:50
When two target names are the same, `mappend`ing them should not
error but just pick the first name.
* Add test for haskell#7423

i.e. Do not warn on -O2 if under off-by-default package configuration
flag conditional.

* Add a regression for:

    * Add another -WErrr test
        This is to make sure we do *not* report it if it is under
        a user, off-by-default flag.
    * Add test for non manual user flags.
    * Add “absolute path in extra-lib-dirs” test
    * Add if/else test
    * Add “dircheck on abspath” check
    * Add Package version internal test
    * Add PackageVersionsStraddle test
* Integrate Artem’s review

(review) Clarify `combineNames` documentation

By explaining the way it operates (working if the two names are equal
or one is empty) and renaming the function from `combineName` to
`combineNames`.

(review) Use guards instead of if/then/else

(review) Match inside argument list

(review) Replace “white” with “allow”

(review) Fix typo in comment

(review) Fix typo in Check module documentation

(review) Harmonise indentation for `data` decls

First field goes in a new line than the data constructor, so we
have more space.

(review) Rename `Prim` module to `Types`

(review) Add checkPackageFilesGPD

`checkPackageFiles` — which works on PD — was used to perform IO. We
introduce a function that does the same thing but works on GPD (which
is more principled).

`checkPackageFiles` cannot just be removed, since it is part of the
interface of Distribution.PackageDescription.Check. Deprecation can
be planned once “new check” is up and running.

* Integrate Andreas’ review

(review) Add named section to missing upper bound check

“miss upper bound” checks will now list target type and name (“On
executable 'myexe', these packages miss upper bounds”) for easier
fixing by the user.

(review) remove `cabal gen-bounds` suggestion

Reasonable as `cabal gen-bounds` is stricter than `cabal check`, see
haskell#8427 (comment)
Once `gen-bounds` behaves in line with `check` we can readd the
suggestion.

(review) Do not warn on shared bounds

When a target which depends on an internal library shares some
dependencies with the latter, do not warn on upper bounds.

An example is clearer

    library
     build-depends: text < 5
    ⁝
     build-depends: myPackage,        ← no warning, internal
                    text,             ← no warning, shared bound
                    monadacme         ← warning!

* Integrate Artem’s review /II

(review) Split Check.hs

Check.hs has been split in multiple file, each une sub 1000 lines:

Check              857 lines
Check.Common       147 lines
Check.Conditional  204 lines
Check.Monad        352 lines
Check.Paths        387 lines
Check.Target       765 lines
Check.Warning      865 lines

Migration guide:
- Check              GPD/PD checks plus work-tree checks.
- Check.Common       common types and functions that are
                     *not* part of monadic checking setup.
- Check.Conditional  checks on CondTree and related matter
                     (variables, duplicate modules).
- Check.Monad        Backbone of the checks, monadic inter-
                     face and related functions.
- Check.Paths        Checks on files, directories, globs.
- Check.Target       Checks on realised targets (libraries,
                     executables, benchmarks, testsuites).
- Check.Warning      Datatypes and strings for warnings
                     and severities.

(review) remove useless section header

(review) Fix typo

(review) Add warnings documentation (list)

For each warning, we document constructor/brief description
in the manual.  This might not be much useful as not but it
will come handy when introducing `--ignore=WARN` and similar
flags.

* (review Andreas) Clarify CheckExplanation comment

Whoever modifies `CheckExplanation` data constructors needs to be
aware that the documentation in  doc/cabal-commands.rst  has to be
updated too.
No need to expose Distribution.PackageDescription.Check.*
to the world. API for checking, for cabal-install and other
tools, should be in Distribution.PackageDescription.Check.
Cabal codebase has now a formatter/style standard (see haskell#8950).

“Ravioli ravioli, give me the formuoli”
See haskell#8963 for reason and clarification requests.
It is now in the Reader part of CheckM monad.
Internal: testsuite, benchmark.
See haskell#8361.
When checking internal version ranges, we need to make sure we
are not mistaking a libraries with the same name but from different
packages. See haskell#9132.
neither…nor, completing what done in haskell#9162
Brandon’s review/II.
@ffaf1
Copy link
Copy Markdown
Collaborator Author

ffaf1 commented Nov 13, 2023

Contributing to cabal is a fullfilling experience! You can have an immediate impact on the ecosystem, people are helpful, thanks everyone for their support!

@andreasabel
Copy link
Copy Markdown
Member

@ffaf1 Congrats on the successful merge, and also on your persistence! I am glad to see this project completed.
It was more than a Summer of Code, more like a Summer Plus Another Year of Code (SpaYoC). It is great you lived it out.

@ulysses4ever
Copy link
Copy Markdown
Collaborator

Reimplementing cabal check, closes #8211, #652, #2065.

#652 wasn't auto-closed. Should it be?

@ffaf1
Copy link
Copy Markdown
Collaborator Author

ffaf1 commented Nov 16, 2023

Thanks, I closed that manually!

@ffaf1 ffaf1 mentioned this pull request Nov 13, 2023
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

attention: needs-manual-qa PR is destined for manual QA cabal-install: cmd/check merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days meta: big squash+merge me Tell Mergify Bot to squash-merge tested-on: windows QA has been successful on Windows

Projects

Status: To Test

7 participants