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

CONTRIBUTING: Package bumps should reference release notes #200922

Merged
merged 1 commit into from
Nov 16, 2022

Conversation

mweinelt
Copy link
Member

@mweinelt mweinelt commented Nov 12, 2022

Release notes for package upgrades are immensely helpful in judging whether the changes made to a package during a version bump are sensible, sufficient or complete.

This change clarifies that a good commit message should contain such a reference.

cc Mic92/nix-update#87

Description of changes
Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 12, 2022
CONTRIBUTING.md Outdated
@@ -51,7 +51,7 @@ See the nixpkgs manual for more details on [standard meta-attributes](https://ni

In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand *why* a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.

For package version upgrades and such a one-line commit message is usually sufficient.
For package version upgrades a simple commit message indicating the old and new version with a reference to the release notes or changelog are sufficient.
Copy link
Member

Choose a reason for hiding this comment

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

It is not quite clear that the headline is a must and the changelog is a should.

Copy link
Member Author

@mweinelt mweinelt Nov 12, 2022

Choose a reason for hiding this comment

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

I should probably change that to must be given, if one exists. Thanks for the pointer. 😛

Copy link
Member

Choose a reason for hiding this comment

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

The meta.changelog could be referenced for such things.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sure. Now only tools like nix-update and nixpkgs-update need to catch up in that regard.

@SuperSandro2000
Copy link
Member

Can't we do a bot that fixes those up if a changelog exists?

Also people are going to chase me out of town with torches if I request this from them while reviewing.

@mweinelt
Copy link
Member Author

Well, here's two things in response to that:

  • Having someone review the release notes on package bumps is important and providing a reference increases the likelihood of that happening.
  • Putting the burden of retrieving the release notes for arbitrary package bump on the reviewer is something I strongly disagree with.

@SuperSandro2000
Copy link
Member

Having someone review the release notes on package bumps is important and providing a reference increases the likelihood of that happening.

If we would make the changelog URL static without the version in it, then it would be super easy to get to the changelog regardless if people manually put it into the commit body or used tools that do that.

Copy link
Member

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

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

Some suggestions:

  • For simple version updates/upgrades, the ones that merely changes version and checksum, a one-line commit and a link to the changelog (when possible - some projects do not have changelog files) should suffice.
  • Things that change the whole logic of Nix expression - such like "the build system changed from autotools to meson" - should be included in the commit messages.

@mweinelt
Copy link
Member Author

Having someone review the release notes on package bumps is important and providing a reference increases the likelihood of that happening.

If we would make the changelog URL static without the version in it, then it would be super easy to get to the changelog regardless if people manually put it into the commit body or used tools that do that.

Ideally meta.changelog gains more popularity, so we can offload the commit messages mostly to tools like nix-update and nixpkgs-update.

This change improves the recommendation for good commit messages to
include release notes on package bumps.

Including the release notes increases the likelihood that they will be
taken into consideration during review. Having them included in the
review is important to be able to judge whether changes made during a
version bump are sensible, sufficient or complete.

The burden of retrieving the release notes for arbitrary package bumps
should not rest on the relatively small group of reviewers.

Signed-off-by: Martin Weinelt <[email protected]>
@mweinelt mweinelt force-pushed the contributing-pkg-bump-rls-notes branch from 21db2bf to 8e4f503 Compare November 14, 2022 16:27
@mweinelt
Copy link
Member Author

Updated the wording taking some feedback into consideration. PTAL.

@piegamesde piegamesde merged commit 649c01e into master Nov 16, 2022
@piegamesde piegamesde deleted the contributing-pkg-bump-rls-notes branch November 16, 2022 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants