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

buildDotnetPackage/fetchNuGet: use pname instead of name and baseName #145090

Merged
merged 2 commits into from
Dec 23, 2021

Conversation

Stunkymonkey
Copy link
Contributor

@Stunkymonkey Stunkymonkey commented Nov 8, 2021

Motivation for this change

follow up of #144949

for buildDotnetPackage the variable baseName was renamed to pname.
i found, the naming of fetchNuGet use name and version. therefore i decided to rename it to pname & version.

sadly this PR turned out to be larger, then expected

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.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.

@SuperSandro2000
Copy link
Member

At some point we should cleanup all those extra fetchNugetDeps functions.

@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch from f3c6c77 to 0f8d554 Compare November 11, 2021 21:33
@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch 4 times, most recently from 5f798b1 to 37910cb Compare November 25, 2021 14:46
@Stunkymonkey Stunkymonkey requested a review from figsoda November 25, 2021 16:12
@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch 4 times, most recently from 43ef890 to 8f7e21a Compare December 3, 2021 23:14
@Stunkymonkey
Copy link
Contributor Author

can this be merged to master, or should i rebase it to staging?

@Stunkymonkey Stunkymonkey requested a review from prusnak December 22, 2021 21:56
@prusnak
Copy link
Member

prusnak commented Dec 22, 2021

can this be merged to master,

Let's aim for master. There are not a lot of rebuilds (11-100). Please rebase and resolve the conflicts.

@IvarWithoutBones can you please have a look? what do you think about this change?

@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch from 15dc4ac to 8be6d94 Compare December 23, 2021 00:37
@Stunkymonkey
Copy link
Contributor Author

I removed the two commits about wasabibackend&github-runner and will adress the improvments in a separate PR, because this PR has a long diff.

Copy link
Member

@IvarWithoutBones IvarWithoutBones left a comment

Choose a reason for hiding this comment

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

LGTM, i think this makes buildDotnetPackage derivations look a lot cleaner, I just have 1 (kind of unrelated) suggestion :)

Thanks for the ping btw! I appreciate it.

pkgs/build-support/build-dotnet-module/default.nix Outdated Show resolved Hide resolved
@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch 2 times, most recently from 60f1547 to 3b2b384 Compare December 23, 2021 21:42
@Stunkymonkey Stunkymonkey force-pushed the buildDotnetPackage-pname branch from 3b2b384 to 81eb599 Compare December 23, 2021 21:47
@Stunkymonkey Stunkymonkey merged commit e0f8595 into NixOS:master Dec 23, 2021
@IvarWithoutBones
Copy link
Member

Thanks!

legendofmiracles added a commit to legendofmiracles/nixpkgs that referenced this pull request Jan 10, 2022
@Stunkymonkey Stunkymonkey deleted the buildDotnetPackage-pname branch February 14, 2022 20:52
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.

4 participants