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

doc/stdenv/meta.chapter.md: explain difference between broken and badPlatforms #225272

Merged
4 commits merged into from Apr 24, 2023
Merged

doc/stdenv/meta.chapter.md: explain difference between broken and badPlatforms #225272

4 commits merged into from Apr 24, 2023

Conversation

ghost
Copy link

@ghost ghost commented Apr 8, 2023

Description of changes

There has been a longstanding ambiguity between broken and badPlatforms, which seem to serve overlapping purposes.

This commit adds to the documentation two examples of constraints which cannot be expressed by platforms and badPlatforms.

This commit also mentions NIXPKGS_ALLOW_BROKEN=1 for overriding broken.

Things done

@ghost ghost requested a review from fricklerhandwerk as a code owner April 8, 2023 07:46
@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 Apr 8, 2023
Copy link
Contributor

@thiagokokada thiagokokada left a comment

Choose a reason for hiding this comment

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

Please amend all your subsequent commits in the first.

…Platforms

There has been a longstanding ambiguity between `broken` and
`badPlatforms`, which seem to serve overlapping purposes.

This commit adds to the documentation two examples of constraints
which cannot be expressed by `platforms` and `badPlatforms`.

This commit also mentions `NIXPKGS_ALLOW_BROKEN=1` for overriding
`broken`.
@ghost
Copy link
Author

ghost commented Apr 11, 2023

Please amend all your subsequent commits in the first.

Done.

@ghost ghost requested a review from thiagokokada April 11, 2023 20:29
Copy link
Contributor

@fricklerhandwerk fricklerhandwerk left a comment

Choose a reason for hiding this comment

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

Thanks, yet again great stuff. Also makes me a bit sad that conventions seem to be all over the place – but that's one point of improved documentation: making such warts more visible and thus easier to address. Related: #140325

doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved
doc/stdenv/meta.chapter.md Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/tweag-nix-dev-update-47/27387/1

Adam Joseph and others added 3 commits April 24, 2023 04:30
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@ghost ghost merged commit 8669061 into NixOS:master Apr 24, 2023
@ghost ghost deleted the pr/docs/broken-vs-badPlatforms branch April 24, 2023 05:09
@zowoq
Copy link
Contributor

zowoq commented Apr 24, 2023

The commits in this PR don't follow the commit format in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md

@zowoq
Copy link
Contributor

zowoq commented Apr 24, 2023

@ghost
Copy link
Author

ghost commented Apr 24, 2023

Same in https://github.com/NixOS/nixpkgs/pull/208848/commits.

nmh: init at 1.7.1

@ghost
Copy link
Author

ghost commented Apr 24, 2023

The commits in this PR don't follow the commit format in https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md

(pkg-name | nixos/<module>):

Neither of these applies to documentation updates (except for a few cases where the commit updates the documentation about a specific package or module).

Based on reviewing the commit history, most people seem to be using

path/to/modified-file.ext: description

so I followed that convention.

Do you have a different recommendation?

@zowoq
Copy link
Contributor

zowoq commented Apr 24, 2023

bece493

@zowoq
Copy link
Contributor

zowoq commented Apr 24, 2023

This PR should have been merge as one commit, not four. These are not acceptable commit messages.

Update doc/stdenv/meta.chapter.md
Update doc/stdenv/meta.chapter.md
remove trailing whitespace

@ghost
Copy link
Author

ghost commented Apr 24, 2023

@zowoq, a more helpful comment would have been "please remember to squash before merging".

@zowoq
Copy link
Contributor

zowoq commented Apr 24, 2023

a more helpful comment would have been "please remember to squash before merging".

It is easier for everyone if the PR is rebased and kept in a mergable state as people forget to squash.

Also sometimes squash should be avoided anyway:

https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md#writing-good-commit-messages
Pull requests should not be squash merged in order to keep complete commit messages and GPG signatures intact and must not be when the change doesn't make sense as a single commit. This means that, when addressing review comments in order to keep the pull request in an always mergeable status, you will sometimes need to rewrite your branch's history and then force-push it with git push --force-with-lease.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.has: documentation 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
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants