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

stdenv/check-meta: description may not end in period #123015

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

dotlambda
Copy link
Member

@dotlambda dotlambda commented May 14, 2021

Motivation for this change

in accordance with https://nixos.org/manual/nixpkgs/unstable/#var-meta-description and https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#submitting-changes

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

The main problem I came across was hackage-packages.nix. Maybe we can automatically remove periods (and probably whitespace) from the end of desciptions upon generating it?
cc @NixOS/haskell
The same goes for the packages in pkgs/development/lisp-modules/quicklisp-to-nix-output/split-sequence.nix.
cc @7c6f434c
... and for poetry2nix
cc @adisbladis

Command run:
    sed -i -r 's/(description\s*=\s*)"([^"]*)\."/\1"\2"/g' pkgs/**/*.nix
Manually take care of the remaining cases.
@dotlambda dotlambda force-pushed the check-meta-description-period branch from c705982 to 2638220 Compare May 14, 2021 20:18
@maralorn
Copy link
Member

We certainly can strip dots in cabal2nix. (I mean why wouldn‘t we?) What confuses me is this commit message (that I vaguely remembered) which claims to already have fixed this. But apparently there still are packages with a dot at the end. NixOS/cabal2nix@2a4560c

@dotlambda
Copy link
Member Author

@maralorn https://github.com/NixOS/cabal2nix/blob/e18310f2eae5d2d7b73512b0b75396224fbf76ca/src/Distribution/Nixpkgs/Haskell/FromCabal/Normalize.hs#L36 looks like dots are only removed from the end if there is at most one dot in the description. I see no reason for that restriction.

@7c6f434c
Copy link
Member

dots are only removed from the end if there is at most one dot in the description. I see no reason for that restriction.

Technically speaking, only one dot is «full stop» punctuation, multiple dots are ellipsis, which has some meaning beyond «the sentenceends here»

@dotlambda
Copy link
Member Author

Technically speaking, only one dot is «full stop» punctuation, multiple dots are ellipsis, which has some meaning beyond «the sentenceends here»

That's not the only case though: For example, some descriptions contain the word "tar.gz" end a single period at the end.

I could change the regex to |.*[^.]|.*\.\. to allow multiple dots at the end.

@7c6f434c
Copy link
Member

Hmm, stupid question: will this also apply to external overlays?

@dotlambda
Copy link
Member Author

Hmm, stupid question: will this also apply to external overlays?

Only if they set checkMeta = true or have somthing like ofborg checking them. See https://github.com/nixos/ofborg#running-meta-checks-locally.

@dotlambda dotlambda requested a review from grahamc May 17, 2021 09:20
@stale
Copy link

stale bot commented Jan 6, 2022

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 6, 2022
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants