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

Development shell with a pinned nixfmt #322512

Merged
merged 6 commits into from
Jun 29, 2024
Merged

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jun 25, 2024

Description of changes

This is a decently-sized step towards having a fully nixfmt-formatted Nixpkgs (#322520) 🎉. I'd like others in the @NixOS/nix-formatting to approve this.

This PR makes it such that the existing workflow to check Nix formatting gives clear instructions on how to fix unformatted files.

It does so by introducing a shell.nix file for Nixpkgs that provides users with a pinned nixfmt version that matches CI.

Along with it comes a workflow to check that nix-build shell.nix works on x86_64-linux and aarch64-darwin (the two relevant platforms available in GitHub Actions), such that it doesn't break randomly

This PR introduces 2 new entries in the root of the directory tree (shell.nix and ci for some extra CI bits), exciting!

Things done


This work is sponsored by Antithesis

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil added the backport release-24.05 Backport PR automatically label Jun 25, 2024
@infinisil infinisil force-pushed the pinned-nixfmt branch 3 times, most recently from 9ca305f to 65bc415 Compare June 25, 2024 23:44
@infinisil infinisil mentioned this pull request Jun 26, 2024
7 tasks
@infinisil infinisil marked this pull request as ready for review June 25, 2024 23:46
@infinisil infinisil requested review from Mic92, zowoq and a team as code owners June 25, 2024 23:46
@infinisil infinisil force-pushed the pinned-nixfmt branch 3 times, most recently from db1efeb to 8f2e12e Compare June 25, 2024 23:54
.github/CODEOWNERS Outdated Show resolved Hide resolved
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild labels Jun 26, 2024
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/formatting-team-meeting-2024-06-25/47661/1

@Frontear

This comment was marked as outdated.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Cool!

A few nits and it's good for me.

.github/CODEOWNERS Outdated Show resolved Hide resolved
.github/workflows/check-nix-format.yml Outdated Show resolved Hide resolved
.github/workflows/check-shell.yml Outdated Show resolved Hide resolved
ci/README.md Outdated Show resolved Hide resolved
@infinisil

This comment was marked as outdated.

@Frontear

This comment was marked as outdated.

@peterhoeg

This comment was marked as outdated.

shell.nix Outdated Show resolved Hide resolved
This creates a new directory for all CI support files, will be populated
in future commits.
This is needed such that in the next commit, we can re-use the same
version from a shell.nix, allowing people to have a guaranteed matching
nixfmt version.
This uses the reusable pinned Nixpkgs from the parent commit to create a
shell.nix file for an environment with a pinned nixfmt version.
When some files are not formatted properly, this shows how people can
fix the problem.

This notably uses the shell.nix introduced in the parent commit to
ensure that the nixfmt version matches what CI expects.
@infinisil infinisil force-pushed the pinned-nixfmt branch 2 times, most recently from 0d74a6b to 58f98e4 Compare June 26, 2024 14:18
@infinisil infinisil mentioned this pull request Jun 26, 2024
1 task
@infinisil
Copy link
Member Author

infinisil commented Jun 26, 2024

Since I don't want this PR to focus on the direnv part, I split that into a separate PR that depends on this one: #322650 (also moved all comments about it there)

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/tweag-nix-dev-update-57/47823/1

# https://stackoverflow.com/a/246128
SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

repo=https://github.com/nixos/nixpkgs
Copy link
Member

Choose a reason for hiding this comment

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

-repo=https://github.com/nixos/nixpkgs
+repo=https://github.com/NixOS/nixpkgs

Copy link
Member Author

Choose a reason for hiding this comment

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

GitHub also works with the lowercase version, so I don't think this matters nor worth changing, even if only to set a precedent for less bikeshedding :P

@infinisil
Copy link
Member Author

Since this has been approved by 3 other members of the formatting team, let's go ahead with merging this ;)

@infinisil infinisil merged commit 81a7752 into NixOS:master Jun 29, 2024
29 checks passed
@infinisil infinisil deleted the pinned-nixfmt branch June 29, 2024 22:00
Copy link
Contributor

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-322512-to-release-24.05 origin/release-24.05
cd .worktree/backport-322512-to-release-24.05
git switch --create backport-322512-to-release-24.05
git cherry-pick -x 13599930cb314ca9335fbb9dcb31acc4342f748d b33ac05d043c03cf98397e5f026ee5a93b781199 a70ab58960c254c58c5289b18b769c9d2311dce7 53b517c6851ef52a86e44ea16220d93f88bf9849 d0bebb7d6b86a705f943dc926527400d7ebc144b 58f98e4b1bc487a519b0f57e9723eb2ce223c229

@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/more-official-guidelines-in-regards-to-styling-choices/48407/1

@infinisil
Copy link
Member Author

Manual backport of this is in #326630

@alyssais
Copy link
Member

alyssais commented Aug 5, 2024

It's a shame this has broken nix-shell -A $pkg to get an interactive package building environment. Is there any way we could preserve that, without having to add "default.nix" to the end of the command?

@infinisil
Copy link
Member Author

@alyssais Oh damn, yeah that's something that didn't occur to me in advance. Looks like nix-shell . -A $pkg works, which seems satisfactory to me. But we could also make nix-shell -A pkgs.$pkg work using passthru.pkgs = import ./. in shell.nix.

@alyssais
Copy link
Member

alyssais commented Aug 7, 2024

Learning to add a . isn't too bad, so I think it would only be worth doing something if it didn't break people's workflows at all, but it's still a shame it broke.

@raboof
Copy link
Member

raboof commented Oct 29, 2024

@alyssais Oh damn, yeah that's something that didn't occur to me in advance. Looks like nix-shell . -A $pkg works, which seems satisfactory to me.

Does it? It doesn't seem to for me (CppNix 2.24.9). nix-shell default.nix -A $pkg or nix-shell '<nixpkgs/default.nix>' -A $pkg does work.

@alyssais
Copy link
Member

Yeah, it got even worse with Nix 2.24: NixOS/nix#11699

@roberth
Copy link
Member

roberth commented Nov 2, 2024

nix#11699 does fix five reported issues, so it would be unfortunate to regress those now.
While I'm open to refining the new rules for nix-shell/shell.nix, they would almost certainly make the final logic more complicated and harder to understand, but probably only fix half of this nix-shell -A issue.

For these reasons I think it is preferable to fix it completely on the Nixpkgs side

@roberth roberth mentioned this pull request Nov 2, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: policy discussion 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild 10.rebuild-linux: 0 This PR does not cause any packages to rebuild backport release-24.05 Backport PR automatically
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.