-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
treewide: format files generated by nuget-to-nix with nixfmt and make code generation of nuget-to-nix follow nixfmt rules #325053
Conversation
d86e4c4
to
8cbdfb2
Compare
8cbdfb2
to
a763a6c
Compare
2c35191
to
f617165
Compare
f617165
to
d447620
Compare
Have you tried running some fetch-deps to test that they match? Obviously that's a little tricky since it can update dependencies, but you could at least make sure nixfmt is a no-op afterwards.
Is this something that nixfmt is always opinionated about? It seems like an annoying quirk to deal with for generated expressions. |
Here's some small script to test regenerating some deps.nix files: pkgs=(
"famistudio"
"galaxy-buds-client"
"openutau"
"tone"
"btcpayserver"
"nbxplorer"
"wasabibackend"
"denaro"
"pinta"
"ArchiSteamFarm"
"avalonia-ilspy"
)
for pkg in "${pkgs[@]}"; do
$(nix-build -A $pkg.fetch-deps)
done At a first glance, the only change seem to be that recently But the formatting looks correct everywhere |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Genererated files are generally excempted from formatters and doing this here just blows up the repository by 40k lines. 👎🏼 from me
Any idea how we make this happen, particularly so it doesn't complain about newly added files? see #331150 (comment) |
RFC166 explicitly makes no decision on whether generated files should be formatted:
Personally, I like the simplicity of just having all nix files formatted. But I can understand why you may not want this.
The RFC's implementation tracking issue currently makes no mention of generated files at all. #322537 (treewide reformat) also does not address how generated files should be handled. I think it is currently assumed they will be formatted. The CI added in #326407 currently enforces formatting for any new files and also any files that were previously formatted. This is skipped if the PR title contains Perhaps this section could be extended to ignore files that contain some "This file is auto-generated" or "skip formatting" style header? nixpkgs/.github/workflows/check-nix-format.yml Lines 75 to 80 in 2e62fe6
cc @infinisil @NixOS/nix-formatting |
I brought this up with the formatting team and we discussed a few ways forward. In the end the consensus seems to be that generated files should almost always use a format such as JSON or TOML, rather than generating nix code. This quote from @emilazy nicely summarises the consensus:
This should be a fairly simple change for dotnet. I believe we would have to update two places; the Plus replacing any existing
Presumably, even if all We'd essentially be doing something like @corngood am I missing anything important here? |
I'm not necessarily against using JSON for this. I decided to use JSON when generating metadata for the VMR releases, see e.g. However, I don't really understand the objection to generating nix. I also don't see why we wouldn't have format checking for JSON, or whatever other formats end up in the repo. In that case we'd be back in the same situation.
Is there a reason we'd care about the number of lines? What is it in absolute/% bytes? What would it be with JSON? |
I don't think there's a strong objection to generating nix, however it was noted that importing nix can be slower than importing JSON/TOML and that it is almost always unnecessary to generate nix, since a data-format such as JSON should be able to represent anything generated automatically. I think the stronger argument is that because generated files can be JSON, there is less motivation to provide a way to avoid formatting generated nix files.
It's hard to say whether non-nix files such as JSON would ever be formatted in the future. Currently the view seems to be it's better to not format data files, since we can usually generate them in a bespoke way that minimizes diff-size and merge conflicts. Quoting @emilazy:
If you're able to join the nix formatting matrix room, the discussion started here, and mostly stayed on topic.
Personally I don't see this as an issue. Potential for merge conflict is far more important than line count. Absolute filesize should be similar with the current formatting vs with nixfmt, I'd expect it to be slightly higher. When mentioned in the matrix discussion, the formatting team also didn't seem concerned about line count. I see three potential paths forward:
As codeowner, your opinion probably matters most in deciding which path to take. |
This might be the issue to follow: NixOS/nixfmt#91
This is my preference for now. When diffing the existing deps files, I almost always use
I have a couple of changes in mind that make me wary about doing this right now:
There's also |
Well, we increase the file size by 9% for nothing.
Also #327064
That could be done and would probably be fine. I think if we do that, we should do it in one treewide commit to avoid having to create any compatibility code.
After any merge conflict the lock files need to be regenerated because it is not guaranteed that merging them by hand will yield the result you need and can only be proofed when building the package. They should be reviewable though, so avoiding having all on one line is still important.
We have a big problem with nixpkgs getting to big and making |
Having seen the recent discussions of lockfile generation, I do think that this PR should not get merged. I'll leave it as draft until there's a better solution. |
I was a bit confused why the file size would change at all but, now that I think about it, that might be due to the added newlines. Your argument however is not sound however because anyone who cares even a little about file sizes will use compression where this increase in newline chars does not matter:
That's a whopping 13 bytes difference. With btrfs' transparent filesystem compression at
Git uses gzip on everything it stores and transmits AFAIK. The only place where this would have an effect that is even measurable would be nixpkgs checkouts in filesystems that do not support transparent compression. Given that such users don't use filesystem compression that IME usually saves you 30-50% of total disk usage in a typical desktop system, I highly doubt they care about something on the order of a 9% increase in a subset of their nixpkgs checkout that amounts to maybe a dozen MiB total at worst. We should definitely prioritise using JSON or TOML over Nix but we don't need to worry or care about file size increases due to formatting either. OTOH I also question whether we need auto-generated Nix files to follow formatting rules at all since no human is supposed to read or modify them in the first place. |
It's not that generated files need to be formatted, rather the issue is that allowing them to not be formatted is additional unnecessary complexity. It's much simpler to say "all nix files must be formatted" rather than "all nix files must be formatted, unless..." |
Please continue further discussion on how to move forward on #358025 |
Description of changes
This PR is made in anticipation of the upcoming treewide reformat of nixpkgs with the new
nixfmt
implementation.The main change is the first commit, that makes code generated by
nuget-to-nix
be formatted hownixfmt
would format it.Please note, that the current logic is not completely correct, since code generation doesn't handle the case of an empty list correctly. I think this should be addressed before this gets merged.
The second commit is not strictly necessary, since, we're gonna get the total treewide formatting anyways, but I think it should be done to avoid bigger diffs when the files get regenerated next.
The second commit was created by running the following bash command:
(where the
nixfmt
is the one provided by thenixfmt-rfc-style
package)I am not sure if the second commit should go into
.git-blame-ignore-revs
.I doubt we really care about git blaming these generated lines.
Note: there's another generator script in
pkgs/development/compilers/dotnet/update.sh
but I won't touch that in this PR.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.