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

addNuGetDeps: support loading JSON lockfiles #362182

Closed
wants to merge 1 commit into from

Conversation

MattSturgeon
Copy link
Contributor

As an initial step away from storing the nuget lockfiles as nix code, this PR makes addNuGetDeps more flexible about the input format.

Specifically, it now imports nix, TOML, and JSON files as required. Entries in the list can either be derivations or arguments applied to fetchNupkg, which coerces them to derivations.

This PR does not touch the output generated by fetch-deps and nuget-to-nix. See #358025. I probably won't have time to work on that side of things either.

I've added some tests, but they're rather limited due to the lockfiles being empty. I'm open to suggestions for more robust test-cases.

cc @corngood @GGG-KILLER

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 6.topic: dotnet Language: .NET labels Dec 5, 2024
@nix-owners nix-owners bot requested a review from corngood December 5, 2024 19:47
@GGG-KILLER
Copy link
Contributor

GGG-KILLER commented Dec 5, 2024

I think this is a good base to start building upon, however I wouldn't merge this as a PR on its own as there's no easy way to convert a nix-based lockfile to TOML or JSON.

Would it be okay if I based myself on this to build the full JSON-based lockfile system?

@MattSturgeon MattSturgeon marked this pull request as draft December 5, 2024 20:07
@MattSturgeon
Copy link
Contributor Author

MattSturgeon commented Dec 5, 2024

I wouldn't merge this as a PR on its own as there's no easy way to convert a nix-based lockfile to TOML or JSON.

I guess that's reasonable. I'll mark this as a draft then.

Would it be okay if I based myself on this to build the full JSON-based lockfile system?

Of course!

@MattSturgeon
Copy link
Contributor Author

I've added some tests, but they're rather limited due to the lockfiles being empty. I'm open to suggestions for more robust test-cases.

Seems the tests didn't commit, so I'll re-push. Re-running them again locally has reminded me that TOML doesn't support toplevel arrays.

We can either drop toml support or have the array be under some key. Dropping support is probably best since we only really need json.

In addition to loading nix lockfiles
@MattSturgeon MattSturgeon changed the title addNuGetDeps: support loading JSON or TOML lockfiles addNuGetDeps: support loading JSON lockfiles Dec 5, 2024
@GGG-KILLER
Copy link
Contributor

Yeah, dropping TOML is probably for the best. I prefer its syntax but JSON has wider support overall.

@ofborg ofborg bot added 8.has: clean-up 8.has: package (new) This PR adds a new package labels Dec 6, 2024
@MattSturgeon MattSturgeon deleted the nuget-json branch December 18, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants