Skip to content

Revert ".devcontainer: nixfmt-rfc-style -> nixfmt"#431258

Merged
wolfgangwalther merged 2 commits intoNixOS:masterfrom
MattSturgeon:revert-430330-push-rumtomvwqmqw
Aug 6, 2025
Merged

Revert ".devcontainer: nixfmt-rfc-style -> nixfmt"#431258
wolfgangwalther merged 2 commits intoNixOS:masterfrom
MattSturgeon:revert-430330-push-rumtomvwqmqw

Conversation

@MattSturgeon
Copy link
Contributor

@MattSturgeon MattSturgeon commented Aug 5, 2025

Reverts #430330

As per #425583 and #430330 (comment), I believe this was bumped prematurely.

Unless the devcontainer is using the in-tree copy of nixpkgs, the CI-pinned revision, or some other known-to-be up-to-date revision... If we can guarantee the devcontainer always uses an up-to-date nixpkgs, then this can be closed.

If the devcontainer is using nixpkgs from (e.g.) the NIX_PATH, then we don't know whether pkgs.nixfmt is the rfc-style formatter or the classic version. That's because NIX_PATH may contain an old revision or a stable release.

@MattSturgeon MattSturgeon requested review from a team and drupol August 5, 2025 20:36
@MattSturgeon MattSturgeon moved this to Todo in Nix formatting Aug 5, 2025
@nixpkgs-ci nixpkgs-ci 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 Aug 5, 2025
Copy link
Contributor

@jfly jfly left a comment

Choose a reason for hiding this comment

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

I know nothing about how devcontainers are used here, but this seems fine to me.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 5, 2025
The alias should be dropped only once `pkgs.nixfmt` is unlikely to be
nixfmt-classic.

I.e. after nixfmt 1.0 is in a stable release, but before the
nixfmt-rfc-style alias starts warning.

See NixOS#425583
@MattSturgeon MattSturgeon requested review from drupol, infinisil and wolfgangwalther and removed request for drupol August 6, 2025 18:36
@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels Aug 6, 2025
Copy link
Contributor

@wolfgangwalther wolfgangwalther left a comment

Choose a reason for hiding this comment

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

I know nothing about .devcontainer either, but nixfmt-rfc-style seems like the safe choice as long as it still exists.

@wolfgangwalther
Copy link
Contributor

It seems like this can be changed whenever the nixfmt in the container at https://github.com/devcontainers/features/pkgs/container/features%2Fnix is the same as nixfmt-rfc-style. Since the last release was 4 months ago, this is unlikely to be the case right now.

@nixpkgs-ci nixpkgs-ci bot added 12.approvals: 3+ This PR was reviewed and approved by three or more persons. and removed 12.approvals: 2 This PR was reviewed and approved by two persons. labels Aug 6, 2025
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented Aug 6, 2025

It seems like this can be changed whenever the nixfmt in the container at https://github.com/devcontainers/features/pkgs/container/features%2Fnix is the same as nixfmt-rfc-style.

If so, should I amend the TODO comment to reference that instead of "a stable release"?

@wolfgangwalther
Copy link
Contributor

I'm not 100% sure about my statement. I looked at the source code for that container a bit, and I'm not sure from which nixpkgs revision it pulls packages. Might as well just be the current latest unstable - no idea.

@wolfgangwalther
Copy link
Contributor

We will only find out if somebody actually uses this thing. With nixfmt, the failure-mode will be much more confusing, because it would result in wrong formatting. With nixfmt-rfc-style the failure mode, once this attribute is removed, will be much more obvious - and once somebody hits this, we can fix it.

Thus, I'm merging this to be on the safe side.

@wolfgangwalther wolfgangwalther merged commit 2ca4ae7 into NixOS:master Aug 6, 2025
29 of 31 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Nix formatting Aug 6, 2025
@MattSturgeon MattSturgeon deleted the revert-430330-push-rumtomvwqmqw branch August 6, 2025 19:47
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. 12.approvals: 3+ This PR was reviewed and approved by three or more persons.

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants