nuget-to-nix: Generated file not formatted

[UlyssesZh]

Describe the bug

The file generated by nuget-to-nix is not formatted. This leads to an error when a generated file is included in a PR and checked by the nixfmt-check step in GitHub Actions.

Some of the ideas that people proposed:

• Modify nuget-to-nix to make the generated file formatted;
• Exempt generated files from nixfmt-check;
• Generate JSON (or alike) instead of Nix.

For details of the pros and cons of those approaches, read the discussions below here and in #325053.

For people who are packaging dotnet applications and meet with nixfmt-check failing: before a long-term solution is decided, temporarily we just merge PR with the check failing anyway (#360598 (comment)). There is no need to run nixfmt in the updater script.

Notify maintainers

@NixOS/dotnet

---

Note for maintainers: Please tag this issue in your PR.

---

Add a 👍 reaction to issues you find important.

[TomaSajt]

Related discussion at: #325053

[GGG-KILLER]

@corngood any apprehensions about converting the output of fetch-deps to JSON?
Iirc we don't have anything that's actually dynamic in it so it should be a 1:1 conversion.
I plan to do it if you give the ok.

About the points you were worried about:

1. Mirror support: we could have an array of urls where we can get the package from instead of a single URL.

2. RIDs could be added as an attribute and we filter through their value (or absence) when building.

[corngood]

I just think it's the wrong way to fix the nixfmt-check problem. JSON might be the right tool for the job, but that's not really why we're choosing it. We're choosing it because it's not checked. If someone decides to add json formatting and checks, we'll be right back in the same place.

I think it would be better to just make fetch-deps format the files. Some packages already do this in their update scripts.

[GGG-KILLER]

I just think it's the wrong way to fix the nixfmt-check problem. JSON might be the right tool for the job, but that's not really why we're choosing it. We're choosing it because it's not checked. If someone decides to add json formatting and checks, we'll be right back in the same place.

I think it would be better to just make fetch-deps format the files. Some packages already do this in their update scripts.

The thing is that we're generating nix code without even needing it, all of our data is completely static and doesn't actually need to be nix, it's pretty much a data file the same way the SDK info files are JSON.

The standard for lockfile generation is to use whatever the language uses, or in the absence of it, JSON or YAML afaik.
We're literally generating a lockfile but in nix without needing it, we don't use any of the nix features or anything that actually requires us to output code instead of plain data.
I'd say it's more about fixing a wrong choice instead of carrying on with it.

[MattSturgeon]

JSON might be the right tool for the job, but that's not really why we're choosing it.

I don't see "avoiding formatting" as the reason to choose JSON.

I see auto-formatting as the prompt to have this discussion in the first place; but the reason to choose JSON is that it is a more appropriate language.

Even if JSON was also going to be auto-formatted, it would still be the better language. However, as things stand now there is little motivation to have data files auto-formatted, so hopefully that's a non-issue.

[corngood]

I just don't really see what we gain from switching. We'll have to break things and/or deal with compatibility.

Performance is a potential benefit, but I haven't benchmarked it, and the only discussion I can find about it is here: https://discourse.nixos.org/t/parser-performance-json-vs-nix/20237/3

[GGG-KILLER]

I just don't really see what we gain from switching. We'll have to break things and/or deal with compatibility.

What we gain is using the right tool for the job, data is not meant to be stored as code.
Lockfiles are data and therefore shouldn't be code files.

That's my stance on it at least. In practice we don't actually gain anything, but keeping it like this just because of sunken cost while other nixpkgs builders were either correct from the start or have fixed this already feels like we are falling for the sunken cost fallacy.

[UlyssesZh]

I read the discussions on this topic and updated the issue description with some summary of the discussions for people who may land on this issue in the future.

[corngood]

In #361579 I have a change to run nixfmt by default after nuget-to-nix. i.e.

Modify nuget-to-nix to make the generated file formatted;

I know there are objections to that doing that, but to me the most pressing problem is described above:

For people who are packaging dotnet applications and meet with nixfmt-check failing: before a long-term solution is decided, temporarily we just merge PR with the check failing anyway (#360598 (comment)). There is no need to run nixfmt in the updater script.

It's tedious having to dig through the check logs to make sure it's only failing on a deps file.

I would still prefer "Exempt generated files from nixfmt-check" as a short term solution, but that doesn't seem to be going anywhere.

I'm also not strongly against the idea of "Generate JSON (or alike) instead of Nix", but I don't see a simple way to do it without breaking compatibility, or maintaining some ugly hacks.

[MattSturgeon]

Short term solution

In #361563 #361579 I have a change to run nixfmt by default after nuget-to-nix. i.e.

Modify nuget-to-nix to make the generated file formatted;

I know there are objections to that doing that, but to me the most pressing problem is described above:

For people who are packaging dotnet applications and meet with nixfmt-check failing: before a long-term solution is decided, temporarily we just merge PR with the check failing anyway (#360598 (comment)). There is no need to run nixfmt in the updater script.

I agree this is a reasonable short term solution. Especially if you don't want to wait for someone to implement json lock files.

Anyone with objections should be reassured by it being temporary, while anyone running into CI failures will have an immediate solution.

Excluding auto-generated files from formatting

I would still prefer "Exempt generated files from nixfmt-check" as a short term solution, but that doesn't seem to be going anywhere.

I don't see this as a viable "short-term" solution, since it is higher effort than #361563 and likely no less effort than converting the nuget lock files to json. It's also seemingly unpopular with the formatting team, who consider it an unnecessary complexity.

JSON lock files

I'm also not strongly against the idea of "Generate JSON (or alike) instead of Nix", but I don't see a simple way to do it without breaking compatibility, or maintaining some ugly hacks.

@SuperSandro2000 seemed ok with this being a breaking change, on the provision that all in-tree lockfiles are updated within a single commit:

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.

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.
From #325053 (comment)

Personally I'd prefer to have it be a non-breaking change, to support out-of-tree users, but I appreciate that adds complexity.

However, I'm not convinced it needs anything too hacky, especially if we can trust the file extension to indicate whether the lockfile is json or nix. Were you thinking of any specific parts of the implementation, that may end up being ugly?

---

I don't think it should be necessary to have a backwards-compatible updater though? Does the updater even get used out-of-tree?

At the most, perhaps auto-removing .nix files that have been replaced with .json by an update script?

Proposed backwards-compatibility importing

I'm imagining a backwards-compatible import could look like:

fetchNuGet: lockfile:
let
  # Detect filetype, using filename suffix
  isJson = lib.hasSuffix ".json" lockfile;

  # Select import vs importJSON
  import' = if isJson then lib.importJSON else import;

  # Load the data, by applying fetchNuGet
  load =
    fnOrList:
    # Legacy impl: pass fetchNuGet as a fn arg
    if builtins.isFunction fnOrList then
      fnOrList { inherit fetchNuGet; }
    # New impl: map over list, applying fetchNuGet
    else builtins.map fetchNuGet fnOrList;
in
load (import' lockfile);

[corngood]

I don't think it should be necessary to have a backwards-compatible updater though? Does the updater even get used out-of-tree?

This is what I was mostly worried about. I've definitely broken people's out-of-tree stuff before with changes to nuget-to-nix, and there have been issues here and there about it.

That being said, I'm sure we could make it result in a clear error with a simple fix (change nugetDeps to a .json file).

[GGG-KILLER]

I think we could use @MattSturgeon's solution to keep it backwards compatible, since in practice all we'll do is pass every element of the JSON array to fetchNuGet anyways, new features available on JSON will also be available on the nix version.

We could add a trace/warning of deprecation on the nix file for a while until we eventually remove it in another release. Doing what Matt suggested was my idea initially to avoid any breaking changes in-tree and off-tree.

[MattSturgeon]

I don't think it should be necessary to have a backwards-compatible updater though? Does the updater even get used out-of-tree?

This is what I was mostly worried about. I've definitely broken people's out-of-tree stuff before with changes to nuget-to-nix, and there have been issues here and there about it.

Perhaps nuget-to-nix can stick around as a deprecated tool and a new nuget-to-json can be introduced to replace it?

I assume most of the implementation could be shared or reused, but having the new tool use a different name might make deprecation and migration easier?

---

I guess this still has potential for breaking changes for out-of-tree users of buildDotnetModule, since passthru.fetch-deps could have an unexpected change in behaviour if it switches from using nuget-to-nix to nuget-to-json...

Maybe fetch-deps could also decide which to use based on file extension, at least for some transition period?

[GGG-KILLER]

Perhaps nuget-to-nix can stick around as a deprecated tool and a new nuget-to-json can be introduced to replace it?

I think we could just rename nuget-to-nix to nuget-to-json and instead make a new nuget-to-nix that just converts the JSON file to a nix file.

I guess this still has potential for breaking changes for out-of-tree users of buildDotnetModule, since passthru.fetch-deps could have an unexpected change in behaviour if it switches from using nuget-to-nix to nuget-to-json...

Maybe fetch-deps could also decide which to use based on file extension, at least for some transition period?

I think we instead could add a flag to the script fetch-deps generates to use nuget-to-nix instead of nuget-to-json and document this change. Caring about out-of-tree users is good but I don't think we should break our backs maintaining 100% compatibility if the way out is easy enough as adding an extra flag to the command (even more so when it'll reduce complexity on our side for something we plan to deprecate anyways).

[MattSturgeon]

One possible approach would be to detect if a deps.nix file exists as a sibling to $1 when doing the final installation of the generated json lockfile...

With something like [ -f "$(dirname "$1")/deps.nix" ] we could a) convert json -> nix (using jq?) and b) print a deprecation warning.

This would only cover updating, rather than attempting to create the initial lockfile having declared nugetDeps = ./deps.nix in buildDotnetModule, however the docs recommend that the initial generation be done without fetch-deps anyway and I think it's fine for new code to have less support for deprecated stuff anyway.

[GGG-KILLER]

One possible approach would be to detect if a deps.nix file exists as a sibling to $1 when doing the final installation of the generated lockfile...

With something like [ -f "$(dirname "$1")/deps.nix" ] we could a) convert json (using jq?) -> nix and b) print a deprecation warning.

I guess we could do this easily enough with

@nugetToJson@/bin/nuget-to-json "${NUGET_PACKAGES%/}" > deps.json

if [ -f "$(dirname "$1")/deps.nix" ]; then
  (
      echo -e "# This file was automatically generated by passthru.fetch-deps.\n# Please dont edit it manually, your changes might get overwritten!\n"
      @nugetToNix@/bin/nuget-to-nix --convert-from deps.json
  ) > deps.nix
  mv deps.nix "$(dirname "$1")/deps.nix"
else
  mv deps.json "$1"
fi

This would only cover updating, rather than attempting to create the initial lockfile having declared nugetDeps = ./deps.nix in buildDotnetModule, however the docs recommend that the initial generation be done without fetch-deps anyway.

That is wrong, we've explicitly made changes so that you can bootstrap a package using fetch-deps and it is the way I've been doing for my out-of-tree packages since forever.

[MattSturgeon]

This would only cover updating, rather than attempting to create the initial lockfile having declared nugetDeps = ./deps.nix in buildDotnetModule, however the docs recommend that the initial generation be done without fetch-deps anyway.

That is wrong, we've explicitly made changes so that you can bootstrap a package using fetch-deps and it is the way I've been doing for my out-of-tree packages since forever.

My mistake, I missed this line when skim-reading:

When writing a new expression, you can use the generated fetch-deps script to initialise the lockfile.

But my main point was that detecting a sibling file would only work if one already exists; i.e. a deps.nix was already generated in the past.

Either way, I don't think it's too important for initialising a new package to also support .nix files. If someone tries to use deps.nix with the new json fetch-deps in a new package they should notice fairly quickly 🤞

If the nugetDeps file doesn't exist, that should produce reasonable error messages already, right?

[GGG-KILLER]

But my main point was that detecting a sibling file would only work if one already exists; i.e. a deps.nix was already generated in the past.

Either way, I don't think it's too important for initialising a new package to also support .nix files. If someone tries to use deps.nix with the new json fetch-deps in a new package they should notice fairly quickly 🤞

I'd consider this a non-issue, if someone really needs to initialize their new package with a .nix file they can go through the effort of converting themselves.

If the nugetDeps file doesn't exist, that should produce reasonable error messages already, right?

I think it does, but can't remember, what I usually do is touch deps.nix, nugetDeps = ./deps.nix and then $(nix-build --no-out-link -A pkg.fetch-deps).

EDIT: It used to, but we changed it to not error on that so that people can bootstrap it even when the file doesn't exist.

[MattSturgeon]

what I usually do is touch deps.nix, nugetDeps = ./deps.nix and then $(nix-build --no-out-link -A pkg.fetch-deps).

Ah, well sibling detection would actually work in that scenario. But I agree, new packages are a non-issue.