Make check comply with Hackage requirements
#8897
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ffaf1
added a commit
to ffaf1/cabal
that referenced
this pull request
Apr 4, 2023
ffaf1
added a commit
to ffaf1/cabal
that referenced
this pull request
Apr 4, 2023
gbaz
approved these changes
Apr 6, 2023
ulysses4ever
approved these changes
Apr 15, 2023
There are two places in cabal codebase where `isDistError` is defined (would Hackage refuse the package based on this error?), plus another one in hackage-server (src/Distribution/Server/Packages/Unpack.hs). This commit refactors the definitions into a singel `isHackageDistError` (in Distribution.Client.Check) so everything is in sync. The function is exported, for the benefit of third-party tools.
`cabal check` exits with 0 or 1. As now the `1` exit is slightly stricter than “Hackage would reject this”, as even PackageDistSuspicious errors will trigger a `1` exit. This patch rectifies this and also specifies the behaviour in the documentation. Notice the usage of “should” and “will”: we cannot be sure Hackage will accept a package (e.g.: maybe there are name clashes) but there are no type I errors: if it fails `cabal check`, Hackage will refuse it.
All the modified tests returned 1 on exit, now they return 0.
(Andreas’ review) For `isHackageDistError`, so that in the future, when constructors are added, the compiler will warn about missing them missing and a thoughtful choice will be made.
(Artem’s review) Saves typing `isHackageDistError` four times more.
(Artem’s review) Make clear that errors will make `check` return 1.
ffaf1
added
squash+merge me
mergify
bot
added
the
merge delay passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
tl;dr:
cabal checkwill exit with1if and only ifHackage would reject the package.As today,
cabal checkhandles reports a “failure” in two distinct ways:Hackage would reject this package.string1. This one is not the same asHackage would reject this package.It is a bit more strict: you could end up withexitcode 1but noHackage would reject this package.string.What this patch does: make the two “failures” bijective. You will get
exitcode 1iifHackage would reject this package.Examples of checks which will not exit with
1anymore:descriptionlonger thansynopsis;-O1usage;maintainer.Check last commit for all of them. If someone has a good reason for them to return
1, let me know (would CI get more annoying to set up? Third-party tools? Maybe it is better to return multiple failure exitcodes?).Towards: #8880
Reviewers: review by commit to skip testsuite changes.
Related from an UX point of view: #8587
QA notes:
cabal checkin any project of yoursHackage would reject this packagein the output.printf '%d\n' $?).1if and only if you see the messageHackage would reject this package.(a quick way to get
Hackage would refuse this packagestring is to removedefault-language: Haskell2010from your.cabal)Specific to this patch:
synopsisto be longer than yourdescription0and you should not see anyHackage would reject this package.messagePlease include the following checklist in your PR:
Tests: I have adjusted the testsuite appropriately.