Skip to content

Treewide cleanup meta description#317959

Merged
mweinelt merged 4 commits intoNixOS:masterfrom
afh:treewide-cleanup-meta-description
Jun 9, 2024
Merged

Treewide cleanup meta description#317959
mweinelt merged 4 commits intoNixOS:masterfrom
afh:treewide-cleanup-meta-description

Conversation

@afh
Copy link
Member

@afh afh commented Jun 7, 2024

Description of changes

  • Remove ending period from meta.description as recommended in section Meta attributes in pkgs.README.md
  • Add recommendation to avoid meta.description to start with the definite or an indefinite article to pkgs/README.md
  • Remove the definite or an indefinite article from the beginning of meta.description

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@mweinelt
Copy link
Member

mweinelt commented Jun 8, 2024

Which command(s) did you use to generate the list?

maintainer/scripts/rebuild-amount.sh --print

@afh
Copy link
Member Author

afh commented Jun 8, 2024

Thanks, @wolfgangwalther, that's very helpful. I'll look into this and make the appropriate changes to this PR.

That's good to know, @mweinelt, thanks! Running ./maintainers/scripts/rebuild-amount.sh --print master | head -6 on this PR branch in a pristine git worktree reports:

Estimating rebuild amount by counting changed Hydra jobs (parallel=unset).
   1 pkgs-lib-tests
   1 release-checks
49808 x86_64-darwin
72937 x86_64-linux

this differs by orders of magnitude from ofborg's and your report.
What am I doing wrong?

@mweinelt
Copy link
Member

mweinelt commented Jun 8, 2024

The base branch has moved on. Try running it with HEAD~4 instead.

@afh afh force-pushed the treewide-cleanup-meta-description branch from 5a98d8a to d42bb33 Compare June 8, 2024 18:42
@afh afh requested review from Ma27, aanderse, drupol and globin as code owners June 8, 2024 18:42
@afh
Copy link
Member Author

afh commented Jun 8, 2024

I'm not sure that's really it as the master branch in my local repo hasn't moved on:

git log --oneline master..HEAD
  d42bb3333f78 (HEAD -> treewide-cleanup-meta-description, origin/treewide-cleanup-meta-descriptio  n) treewide: Improve wording of meta.description
  ce8e26495cf1 treewide: Remove the definite article from meta.description
  84b5eb1d233e treewide: Remove indefinite article from meta.description
  c4182ec375eb treewide: Remove ending period from meta.description

When running ./maintainers/scripts/rebuild-amount.sh --print HEAD~4 the following error is printed:

error: syntax error, unexpected PATH_END, expecting DOLLAR_CURLY

       at /var/folders/h0/948x8bgn0kd7b1d7hf0bnlr00000gp/T/nix-rebuild-amount-FKM2cs8b:2:73:

            1|         let
            2|           lib = import /var/folders/h0/948x8bgn0kd7b1d7hf0bnlr00000gp/T//nix-rebuild-amount-r1Ymbr2y/lib;

@afh
Copy link
Member Author

afh commented Jun 8, 2024

@wolfgangwalther Another pattern I found is a comment = description with a let … in block. I went ahead and updated, i.e. removed those changes to description, the commits in this PR; let's see what ofborg says about this.

Until I'm able to reliably and "correctly" run ./maintainers/scripts/rebuild-amount.sh would you, @mweinelt, or someone else might be so kind to run it on my behalf to see which rebuilds remain?

@mweinelt
Copy link
Member

mweinelt commented Jun 8, 2024

      1 release-checks
      3 x86_64-darwin
     17 x86_64-linux

bitwuzla.x86_64-darwin                                                                   /nix/store/30m205wsdy1naacfffv169yr54khqq7a-bitwuzla-0.5.0
bitwuzla.x86_64-linux                                                                    /nix/store/rkln14w6dkdrn1sk43xczfvaaydvpg99-bitwuzla-0.5.0
cvc5.x86_64-darwin                                                                       /nix/store/45pnm5cjq783ylw7mbzpnja79q7dwhgb-cvc5-1.1.2
cvc5.x86_64-linux                                                                        /nix/store/dak75srlzxkalh9grvc9ji9rnr82zclk-cvc5-1.1.2
experienced-pixel-dungeon.x86_64-linux                                                   /nix/store/kdviyncfx7fp3fn7jmg4k16aq88f1n1r-experienced-pixel-dungeon-2.18
hase.x86_64-linux                                                                        /nix/store/mzx1jcqcxy8lh8l1f82j72z24f64z1zq-hase-unstable-2020-10-06
inochi-creator.x86_64-linux                                                              /nix/store/8mk28pbc22f2wi5m00razrvc9pxyhh56-inochi-creator-0.8.5
inochi-session.x86_64-linux                                                              /nix/store/vh075f3v2m9b24f8l4hrw5xns99dq164-inochi-session-0.8.4
openraPackages_2019.mods.ca.x86_64-linux                                                 /nix/store/4ncrsaavxfy4hqsx7g4kqq2nr523b48b-openra_2019-ca-96.git.fc3cf0b
openraPackages_2019.mods.d2.x86_64-linux                                                 /nix/store/lc0gmi01m257lv935nnzc88y07i1mwh0-openra_2019-d2-134.git.69a4aa7
openraPackages_2019.mods.dr.x86_64-linux                                                 /nix/store/wr5xxq9897bi2dw9g2k11pzpdyr8571k-openra_2019-dr-324.git.ffcd6ba
openraPackages_2019.mods.mw.x86_64-linux                                                 /nix/store/izsg0szly1874qh2pczmv3vrqzcq08ir-openra_2019-mw-257.git.c9be8f2
openraPackages_2019.mods.raclassic.x86_64-linux                                          /nix/store/bb09kxzkxvnnlw29g8qanzvmimzcrr3q-openra_2019-raclassic-183.git.c76c13e
openraPackages_2019.mods.ss.x86_64-linux                                                 /nix/store/bzz5m1snailkg0zjbw4jr6401iz7dmjn-openra_2019-ss-77.git.23e1f3e
rat-king-adventure.x86_64-linux                                                          /nix/store/lr3ikj90g9fffz1havl27avkjisr0g5z-rat-king-adventure-1.5.3
release-checks                                                                           /nix/store/0r800i0f1mmhr48444620p3yzcdpc2i9-nixpkgs-release-checks
shorter-pixel-dungeon.x86_64-linux                                                       /nix/store/cc4h2b8mrgdxj7pkn22375p6wym900g4-shorter-pixel-dungeon-1.4.0
sparrow3d.x86_64-linux                                                                   dev=/nix/store/skrasddkgpp1ciamqgsbgwsqblf20f07-sparrow3d-unstable-2020-10-06-dev;/nix/store/kxwqr8smh8ddl5whyqp0vl5ggfpqm8dy-sparrow3d-unstable-2020-10-06
summoning-pixel-dungeon.x86_64-linux                                                     /nix/store/a11x0wcvk4kmxrw1506y9q17i9zmq0yz-summoning-pixel-dungeon-1.2.5a
symfpu.x86_64-darwin                                                                     /nix/store/mrff1pcxyc8gvdzghx3mqy3ym3mqwrml-symfpu-unstable-2019-05-17
symfpu.x86_64-linux                                                                      /nix/store/s7gdmkqgmmrchk1pdcf0d37a30qhsqsq-symfpu-unstable-2019-05-17

@afh
Copy link
Member Author

afh commented Jun 8, 2024

Very helpful, @mweinelt, thanks a bunch! I addressed the issues I found (see details below), mind generating the list again?

  • bitwuzla and cvc5 have a dependency on symfpu, so if symfpu is built those two packages also get build
  • hase has a dependency sparrow3d
  • symfpu and sparrow3d inherit dependency in pkgconfigItems
  • inochi2d, rat-king-adventure, summoning-pixel-dungeon, shorter-pixel-dungeon, experienced-pixel-dungeon uses comment = description in generic.nix from which the packages are built
  • openraPackages_2019 uses description installPhase with substitute, which likely triggers a rebuild.

@mweinelt
Copy link
Member

mweinelt commented Jun 8, 2024

      1 release-checks

release-checks                                                                           /nix/store/5vmlq9q78vp9xlsvf58ncb4p82274wpx-nixpkgs-release-checks

@afh
Copy link
Member Author

afh commented Jun 8, 2024

Thanks once more, @mweinelt, very much appreciated! 🙏

I'm looking at pkgs/top-level/nixpkgs-basic-release-checks.nix which seems to be what is run as release-checks, but I'm not familiar with those aspects of nixpkgs. Can someone more knowledgable chime in on what changes in this PR could cause release-checks to be rebuilt?

@K900 would you be open to re-consider and potentially give your approval to the changes proposed here with 1 rebuild?

@afh afh mentioned this pull request Jun 9, 2024
13 tasks
@K900
Copy link
Contributor

K900 commented Jun 9, 2024

There's merge conflicts now.

afh added 4 commits June 9, 2024 23:04
nix run nixpkgs#silver-searcher -- -G '\.nix$' -0l 'description.*".*\.";' pkgs \
  | xargs -0 nix run nixpkgs#gnused -- -i '' -Ee 's/(description.*)\.";/\1";/'
nix run nixpkgs#silver-searcher -- -G '\.nix$' -0l 'description.*"[Aa]n?' pkgs \
  | xargs -0 nix run nixpkgs#gnused -- -i '' -Ee 's/(description.*")[Aa]n? (.)/\1\U\2/'
nix run nixpkgs#silver-searcher -- -G '\.nix$' -0l 'description.*"([Tt]he)? ' pkgs \
  | xargs -0 nix run nixpkgs#gnused -- -i '' -Ee 's/(description.*")[Tt]he (.)/\1\U\2/'
@afh
Copy link
Member Author

afh commented Jun 9, 2024

The merge conflicts have been addressed, @K900. Given the amount of changes in this PR and the many contributions to this project it seems there is narrower merge window. I'll do my best too keep this up-to-date and address merge conflicts.

@afh
Copy link
Member Author

afh commented Jun 10, 2024

Thanks everyone for lending your expertise on this PR and helping getting it merged, very much appreciated! 😃🙏

@Aleksanaa
Copy link
Member

Add recommendation to avoid meta.description to start with the definite or an indefinite article to pkgs/README.md

Is there any reason for us to do this? Is there any prior context for adding this rule here?

IMO starting the description with "A", "An" or "The" does not affect the correctness of the description and makes it read more naturally. I don't understand at all why we need to avoid it.

@Aleksanaa
Copy link
Member

Let me give you an example: "An implementation of" and "implementation of" can be interpreted to different meanings. The latter seems even less clear: is this an official/standard/only implementation, or just an implementation?

@Aleksanaa
Copy link
Member

In addition, you should not modify the document and perform treewide changes in one commit. This makes the situation even more unclear to reviewers: although you gave the command, since I found some changes that were obviously not completed by this command, So how do I ensure that all the modifications in your commit (which contains thousands of files and can even freeze Firefox when browsing online) are reliable?

The modification of the document is also incomplete. You have not modified the corresponding part in the nixpkgs manual, which creates more confusion. The manual still describes it as "right": https://github.com/NixOS/nixpkgs/blob/master/doc/stdenv/meta.chapter.md#description-var-meta-description

@afh
Copy link
Member Author

afh commented Jun 15, 2024

Thank you, @Aleksanaa, for chiming in with helpful feedback, very much appreciated, 🙏

Is there any reason for us to do this?

To keep the description succinct and concise and free from words that provide no additional meaning.

Is there any prior context for adding this rule here?

OpenBSD's Porting Guide (see 18.) shares similar recommendations as the the nixpkgs' pkgs/README.md and recommends to "remove the article altogether:"

Add COMMENT in Makefile. COMMENT is a SHORT one-line description of the port (max. 60 characters). Do NOT include the package name (or version number of the software) in the comment. Do NOT start with an uppercase letter unless semantically significant, and do NOT end with a period. DON'T EVER START WITH AN INDEFINITE ARTICLE SUCH AS "a" or "an" - remove the article altogether.

The latter seems even less clear: is this an official/standard/only implementation, or just an implementation?

If it is an official or standard implementation the description should include that adjective, if no such adjective is present one can safely assume it is just an implementation.

you should not modify the document and perform treewide changes in one commit.

So one commit for the doc change and one for the treewide application?

I found some changes that were obviously not completed by this command

Which examples did you find?

how do I ensure that all the modifications in your commit (which contains thousands of files and can even freeze Firefox when browsing online) are reliable

Unfortunately I do not have a good answer for that from the top of my head. Let me give it some thought.

You have not modified the corresponding part in the nixpkgs manual

Apologies for that I thought the pkgs/README.md was the authoritative source and I'll gladly create a follow-up PR to remedy the shortcoming(s) of this PR.

Hopefully I've addressed all your comments and concerns, in case I something left out, please let me know.

@Aleksanaa
Copy link
Member

To keep the description succinct and concise and free from words that provide no additional meaning.

IMO The smooth and less ambiguous grammar should be the first guarantee. Deleting "A/An/THE" does not significantly reduce the time of reading or reduce the size of the repository.

OpenBSD's Porting Guide (see 18.) shares similar recommendations as the the nixpkgs' pkgs/README.md and recommends to "remove the article altogether:"

Okay, that's at least the same rule from somewhere. I just checked and it seems that Debian does not add the A/An/The prefix, while Arch and Fedora do not restrict this. Of course I'm not saying that OpenBSD is the "model distribution", or that Fedora and Arch are. But attitudes on this issue are clearly divided.

I think this needs further discussion. This may have something to do with native language habits. Some other people in the Chinese group also complained to me that removing the prefix Definite/Indefinite article makes no sense. However, I don't know why, given a controversial decision, this PR was merged within a few days without any discussion about it. This is obviously inappropriate.

Which examples did you find?

I mean editing the doc in one commit with the treewide change.

Unfortunately I do not have a good answer for that from the top of my head. Let me give it some thought.

If you just give a command (thanks for giving the command at least), we can directly reproduce the diff. If you throw some other modifications into it (I mean the documentation, other parts haven't been fully checked yet), things get a little more difficult.

@eclairevoyant
Copy link
Contributor

Let me give you an example: "An implementation of" and "implementation of" can be interpreted to different meanings. The latter seems even less clear: is this an official/standard/only implementation, or just an implementation?

"The implementation" does not imply standard impl either. If it's really to imply that it's the standard impl, then put the word "standard" or "reference" in the desc and be completely unambiguous. And to call something "the implementation" is misleading; it might be the only impl at the time... until it's not. I find it very difficult to believe that removing leading articles would impede understanding.

@Aleksanaa Aleksanaa mentioned this pull request Jun 16, 2024
13 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

6.topic: cinnamon Desktop environment 6.topic: dotnet Language: .NET 6.topic: emacs Text editor 6.topic: Enlightenment DE The Enlightenment Desktop Environment 6.topic: erlang General-purpose, concurrent, functional high-level programming language 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: golang Go is a high-level general purpose programming language that is statically typed and compiled. 6.topic: haskell General-purpose, statically typed, purely functional programming language 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 6.topic: k3s Kubernates distribution (https://k3s.io/) 6.topic: Lumina DE The Lumina Desktop Environment 6.topic: LXQt The Lightweight Qt Desktop Environment 6.topic: mate The MATE Desktop Environment 6.topic: pantheon The Pantheon desktop environment 6.topic: php PHP is a general-purpose scripting language geared towards web development. 6.topic: python Python is a high-level, general-purpose programming language. 6.topic: qt/kde Object-oriented framework for GUI creation 6.topic: ruby A dynamic, open source programming language with a focus on simplicity and productivity. 6.topic: rust General-purpose programming language emphasizing performance, type safety, and concurrency. 6.topic: vim Advanced text editor 6.topic: vscode A free and versatile code editor that supports almost every major programming language. 6.topic: xfce The Xfce Desktop Environment 6.topic: zig Zig is an imperative, general-purpose, statically typed, compiled system programming language. 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants