cabal check: Warn if changelogs are omitted from extra-doc-files #8657

wismill · 2023-01-07T19:11:03Z

Fixes #3964.

Based on @sjakobi’s work.

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.
Added test MissingExpectedExtraDocFiles.

Mikolaj · 2023-01-09T08:49:46Z

It seems the OSX runner of CI locked up. I've taken the liberty of restarting failed jobs.

Mikolaj

LGTM

Cabal/src/Distribution/PackageDescription/Check.hs

ffaf1

Thanks, this is a very useful check. As noted in my review, I think this should go in checkPackageFilesPreDistribution, because once the .tar.gz is made/uploaded, then it is too late.

Please tell me if what I say makes sense and whether I can help you in modifying the patch.

Cabal/src/Distribution/PackageDescription/Check.hs

Mikolaj · 2023-01-10T18:23:37Z

BTW, I've just found #8068.

wismill · 2023-01-10T19:53:32Z

BTW, I've just found #8068.

Somehow I missed it 😅. If I understand it well, it is also based on sjakobi’s work but does not modify CheckPackageContentOps. Anyway, it does not solve what @ffaf1 pointed out here either.

There is also a comment of phadej to be addressed:

Warning: These warnings may cause trouble when distributing the package:

Is not correct.

There needs to be even weaker level.

I have packages where README is just for GitHub to not look stupid. All "README" is already in main module documentation or package description.

(ghc-options: -O2 having PackageDistSuspiciousWarn is acceptable, it's a fluke we ought to fix: If cabal-install is compiling with optimizations: False it would be great if packages weren't -O2, but if optimizations are enabled package authors could choose prefer -O2 over -O1 (or some other additional GHC flags). I.e. .cabal format isn't descriptive enough. Missing README isn't that level of trouble.)

@phadej Would it be ok to display the warning for readmes only if there is no long description of the package, or if it is too short¹?

Say, 70 characters or so, to avoid: See the readme on <https://github.com/haskell/cabal Github>. ↩

phadej · 2023-01-10T21:54:21Z

Say, 70 characters or so, to avoid: See the readme on <https://github.com/haskell/cabal Github>. leftwards_arrow_with_hook

I agree with Herbert on the fact that these stack introduced description:s can be just warned about as well. They serve no purpose. Write an abstract about your package. Put the rest of the info (if any) into readme.

That's really a problem with stack new, not with cabal check.

EDIT: Also stack template omitted readmes from extra-source-files, which made packages created with it look quite stupid.

Bodigrim · 2023-01-10T22:53:24Z

+100 to warn on missing changelogs. I can live without README, but changelogs are essential.

wismill · 2023-01-11T07:24:24Z

I agree with Herbert on the fact that these stack introduced description:s can be just warned about as well. They serve no purpose. Write an abstract about your package. Put the rest of the info (if any) into readme.

@phadej I am not sure I follow you: I find description field quite useful. When I look for a package on Hackage:

I get first a short description with synopsis in the list of results,
then I get a good summary with description on the package’s page, without having to scroll down to the readme (if exisisting) nor open the doc pages.

Also, in order to understand the context better, could you link or summarize which discussion with Herbert you refer to?

ffaf1 · 2023-01-11T08:16:53Z

(will repost here for better visibility)

@wismill what is your current idea about this PR? Warning about “file is present in the working tree but not in extra-doc-files” or “missing changelog and readme from extra-doc-file” (irrespective of files present in the working tree)?

If the latter, you can do it by a simple, pure check; the former would not be needed anymore, I think.

wismill · 2023-01-11T08:31:17Z

@wismill what is your current idea about this PR? Warning about “file is present in the working tree but not in extra-doc-files” or “missing changelog and readme from extra-doc-file” (irrespective of files present in the working tree)?

changelog: latter approach. But it will probably break lots of tests.
readme: initially I thought about the former approach, but maybe refined (see my comment): readme may not be necessary if the package has already a good description, or if it is reserved for another use (for example: Github only). Now I am inclined to used the latter approach, if refined.

ffaf1 · 2023-01-11T08:43:49Z

I see, thanks. Let’s see if more people chime in.

re: tests you can mass --accept them. Not the cleanest solution, but it would do.

…istribution

phadej · 2023-01-11T10:22:04Z

@wismill

description: See the readme on <https://github.com/haskell/cabal Github>.

is useless. Proper descriptions (i.e. short abstracts describing the package) are great.

wismill · 2023-01-11T10:54:35Z

@phadej OK then, we agree these descriptions are useless. So if I understand well, these kind of descriptions are generated by stack when it creates a cabal file from a stack.yaml file. Is it correct?

Now, there are two questions to consider:

Has the package a good-enough description field to avoid needing a readme? This is necessary to address your comment about some readmes existing only for repository browsing. This can be solved by requiring for example at least 10 words. This will avoid stack-generated descriptions.
What is a good description field? This is more general and may require another issue to be addressed.

I am addressing 1) in a further commit.

phadej · 2023-01-11T11:07:51Z

This can be solved by requiring for example at least 10 words. This will avoid stack-generated descriptions.

You are really touching a very old s**t pile. The check for descriptions to be longer than synopsis is good enough and not annoying people.

For the record, there are many people who turn e.g. cabal check off in the haskell-ci (or not use it in their CI at all), so making it more annoying (and non configurable) will make more people use cabal check less, which is a net loss.

wismill · 2023-01-11T11:19:36Z

OK then: should we:

Target specifically stack-generated description?
Always ask for readme?
Aks to include a readme only if the file already exists?
Only check for changelog?
New ideas?

I prefer 3 > 4 > 2 > 1

Check only globs, not the filesystem.

Mikolaj

Brilliant. Please kindly set the label (I guess squash+merge_me) and this will give everybody 2 more days to re-review or otherwise comment.

thank you for the review; I presume the changes have been applied, so I'm taking the liberty to unblock merging

phadej

Add tests for cabal-version: 1.12 (i.e. which doesn't yet have extra-doc-files) and adjust the check warnings accordingly.

Mikolaj · 2023-01-16T17:14:51Z

*could you please

wismill · 2023-01-17T06:26:10Z

I adjusted the code for Cabal <= 1.12 and I added label quash+merge_me.

ffaf1

Well done

Mikolaj · 2023-01-21T22:32:00Z

Some random net outage crashed CI, so I restarted affected tests.

Mikolaj · 2023-01-23T08:03:22Z

@mergify refresh

mergify · 2023-01-23T08:03:58Z

refresh

✅ Pull request refreshed

ulysses4ever · 2023-01-24T03:35:36Z

@wismill since the changelog-only version is merged, could you, perhaps, rename the PR and the corresponding issue, to reflect the actual change?

andreasabel · 2023-02-09T10:40:55Z

@wismill: Thanks for this awesome new feature!

I encountered this:

Warning: Please consider including the file './CHANGELOG.md~' in the
'extra-doc-files' section of the .cabal file if it contains useful information
for users of the package.

I think the file filter should exclude common backup file extensions.

Ah, yes, and we should test for this. A single test containing lots of variants for files that should not be considered would suffice here.

CC: @ffaf1

andreasabel · 2023-02-09T10:57:21Z

Cabal/src/Distribution/PackageDescription/Check.hs

+    isDesirableExtraDocFile :: [FilePath] -> FilePath -> Bool
+    isDesirableExtraDocFile paths path = map toLower basename `elem` paths
+      where
+        (basename, _ext) = splitExtension path


Extensions like .bak, tmp, .*~, files ending in ~ etc. should be ruled out here.

Ideally, you would rule out anything in .gitignore as well.

.swp, .swn, swo,...

Almost better to whitelist extensions (no extension, txt, md, rst).

also .markdown

“no extension“ should be whitelisted too, NEWS is common in GNU programs.

See: https://github.com/haskell/hackage-server/blob/master/src/Distribution/Server/Packages/ChangeLog.hs

I would also add rst. PR comming.

Mikolaj · 2023-02-13T16:33:00Z

Am I correct this needs a backport to 3.10?

ulysses4ever · 2023-02-13T17:03:35Z

I'd vote yes: I can't imagine this could backfire. (Although, my imagination is limited!)

Mikolaj · 2023-02-13T17:35:53Z

If I understand this right, our choice is to revert the original change or apply the fix. Keeping the change that we know has problems doesn't make sense to me.

Actually, good that I asked, because I misplaced the comment. I meant to ask this about #8747. Unless I'm mixing everything up and the original change is not on branch 3.10?

wismill force-pushed the wip/check_readme branch from bb20632 to bd91b7f Compare January 7, 2023 20:23

wismill added 2 commits January 8, 2023 12:35

cabal check: Warn if expected files are omitted from extra-doc-files

e4fee32

Add test

eb43ee0

wismill force-pushed the wip/check_readme branch from bd91b7f to eb43ee0 Compare January 8, 2023 11:35

wismill marked this pull request as ready for review January 8, 2023 11:36

Mikolaj approved these changes Jan 9, 2023

View reviewed changes

Cabal/src/Distribution/PackageDescription/Check.hs Outdated Show resolved Hide resolved

Mikolaj requested a review from ffaf1 January 9, 2023 08:57

Mikolaj added attention: needs-review cabal-install: cmd/check labels Jan 9, 2023

Add missing files and their hackage’s reference

cae69e1

ffaf1 previously requested changes Jan 9, 2023

View reviewed changes

Cabal/src/Distribution/PackageDescription/Check.hs Outdated Show resolved Hide resolved

Mikolaj removed the attention: needs-review label Jan 10, 2023

Move doc file check from checkPackageContent to checkPackageFilesPreD…

1da3835

…istribution

wismill force-pushed the wip/check_readme branch from ca9f9e0 to 1da3835 Compare January 11, 2023 08:51

wismill added 2 commits January 11, 2023 18:32

Check based on globs only

5a80e36

Check only globs, not the filesystem.

Update test outputs

ce49487

wismill force-pushed the wip/check_readme branch from b085763 to ce49487 Compare January 11, 2023 17:32

Mikolaj approved these changes Jan 16, 2023

View reviewed changes

phadej suggested changes Jan 16, 2023

View reviewed changes

wismill added 2 commits January 17, 2023 05:49

Remove getGlobFiles

18c5a60

Adapt warnings to version 1.12

4baf6ba

wismill added the squash+merge me Tell Mergify Bot to squash-merge label Jan 17, 2023

ffaf1 approved these changes Jan 18, 2023

View reviewed changes

mergify bot added the merge delay passed Applied (usually by Mergify) when PR approved and received no updates for 2 days label Jan 20, 2023

Merge branch 'master' into wip/check_readme

c17daac

Merge branch 'master' into wip/check_readme

dc8c265

Mikolaj requested a review from joshleeb January 23, 2023 08:21

mergify bot merged commit 6fb48c7 into haskell:master Jan 23, 2023

wismill changed the title ~~cabal check: Warn if readmes or changelogs are omitted from extra-doc-files~~ cabal check: Warn if rea changelogs are omitted from extra-doc-files Jan 24, 2023

wismill changed the title ~~cabal check: Warn if rea changelogs are omitted from extra-doc-files~~ cabal check: Warn if changelogs are omitted from extra-doc-files Jan 24, 2023

This was referenced Jan 24, 2023

cabal check should warn when readme or changelog exist but aren't included in extra-source-files #3964

Open

Warn of readme,changelog not in extra-source-files #8068

Closed

ffaf1 mentioned this pull request Feb 6, 2023

Reimplement cabal check #8427

Merged

4 tasks

andreasabel mentioned this pull request Feb 9, 2023

cabal check wants me to add CHANGELOG.md~ #8746

Closed

andreasabel reviewed Feb 9, 2023

View reviewed changes

wismill mentioned this pull request Feb 13, 2023

cabal-install: check: Whitelist doc file extensions #8747

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cabal check: Warn if changelogs are omitted from extra-doc-files #8657

cabal check: Warn if changelogs are omitted from extra-doc-files #8657

wismill commented Jan 7, 2023 •

edited

Loading

Mikolaj commented Jan 9, 2023

Mikolaj left a comment

ffaf1 left a comment

Mikolaj commented Jan 10, 2023

wismill commented Jan 10, 2023 •

edited

Loading

phadej commented Jan 10, 2023 •

edited

Loading

Bodigrim commented Jan 10, 2023

wismill commented Jan 11, 2023

ffaf1 commented Jan 11, 2023

wismill commented Jan 11, 2023

ffaf1 commented Jan 11, 2023

phadej commented Jan 11, 2023

wismill commented Jan 11, 2023 •

edited

Loading

phadej commented Jan 11, 2023

wismill commented Jan 11, 2023 •

edited

Loading

Mikolaj left a comment

phadej left a comment

Mikolaj commented Jan 16, 2023

wismill commented Jan 17, 2023 •

edited

Loading

ffaf1 left a comment

Mikolaj commented Jan 21, 2023

Mikolaj commented Jan 23, 2023

mergify bot commented Jan 23, 2023

ulysses4ever commented Jan 24, 2023

andreasabel commented Feb 9, 2023 •

edited

Loading

andreasabel Feb 9, 2023 •

edited

Loading

phadej Feb 9, 2023

Mikolaj Feb 9, 2023

ffaf1 Feb 9, 2023

wismill Feb 9, 2023

wismill Feb 9, 2023

Mikolaj commented Feb 13, 2023

ulysses4ever commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

cabal check: Warn if changelogs are omitted from extra-doc-files #8657

cabal check: Warn if changelogs are omitted from extra-doc-files #8657

Conversation

wismill commented Jan 7, 2023 • edited Loading

Mikolaj commented Jan 9, 2023

Mikolaj left a comment

Choose a reason for hiding this comment

ffaf1 left a comment

Choose a reason for hiding this comment

Mikolaj commented Jan 10, 2023

wismill commented Jan 10, 2023 • edited Loading

Footnotes

phadej commented Jan 10, 2023 • edited Loading

Bodigrim commented Jan 10, 2023

wismill commented Jan 11, 2023

ffaf1 commented Jan 11, 2023

wismill commented Jan 11, 2023

ffaf1 commented Jan 11, 2023

phadej commented Jan 11, 2023

wismill commented Jan 11, 2023 • edited Loading

phadej commented Jan 11, 2023

wismill commented Jan 11, 2023 • edited Loading

Mikolaj left a comment

Choose a reason for hiding this comment

phadej left a comment

Choose a reason for hiding this comment

Mikolaj commented Jan 16, 2023

wismill commented Jan 17, 2023 • edited Loading

ffaf1 left a comment

Choose a reason for hiding this comment

Mikolaj commented Jan 21, 2023

Mikolaj commented Jan 23, 2023

mergify bot commented Jan 23, 2023

✅ Pull request refreshed

ulysses4ever commented Jan 24, 2023

andreasabel commented Feb 9, 2023 • edited Loading

andreasabel Feb 9, 2023 • edited Loading

Choose a reason for hiding this comment

phadej Feb 9, 2023

Choose a reason for hiding this comment

Mikolaj Feb 9, 2023

Choose a reason for hiding this comment

ffaf1 Feb 9, 2023

Choose a reason for hiding this comment

wismill Feb 9, 2023

Choose a reason for hiding this comment

wismill Feb 9, 2023

Choose a reason for hiding this comment

Mikolaj commented Feb 13, 2023

ulysses4ever commented Feb 13, 2023

Mikolaj commented Feb 13, 2023

wismill commented Jan 7, 2023 •

edited

Loading

wismill commented Jan 10, 2023 •

edited

Loading

phadej commented Jan 10, 2023 •

edited

Loading

wismill commented Jan 11, 2023 •

edited

Loading

wismill commented Jan 11, 2023 •

edited

Loading

wismill commented Jan 17, 2023 •

edited

Loading

andreasabel commented Feb 9, 2023 •

edited

Loading

andreasabel Feb 9, 2023 •

edited

Loading