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

buildDotnetModule: fix structured attributes support #313005

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

tie
Copy link
Member

@tie tie commented May 19, 2024

Description of changes

This PR contains 3 commits from #291640 that refactor buildDotnetModule internal implementation (and should potentially make it easier to expose hooks for standalone use in the future).

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/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 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.

@ofborg ofborg bot added the 6.topic: cross-compilation Building packages on a different platform than they will be used on label May 19, 2024
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label May 19, 2024
@tie tie changed the title Dotnet cross buildDotnetModule: fix structured attributes support May 19, 2024
@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label May 20, 2024
@tie tie force-pushed the dotnet-cross branch 2 times, most recently from 1503383 to 88df5d2 Compare May 20, 2024 01:12
@tie
Copy link
Member Author

tie commented May 20, 2024

Result of nixpkgs-review pr 313005 run on aarch64-linux 1

1 package marked as broken and skipped:
  • naps2
61 packages built:
  • ArchiSteamFarm
  • BeatSaberModManager
  • am2rlauncher
  • audiobookshelf
  • avalonia-ilspy
  • bicep
  • boogie
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • cyclonedx-cli
  • dafny
  • denaro
  • depotdownloader
  • dotnet-outdated
  • fable
  • famistudio
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • garnet
  • git-credential-manager
  • github-runner
  • gitversion
  • ilspycmd
  • jackett
  • jellyfin
  • kavita
  • knossosnet
  • libation
  • lubelogger
  • lumafly
  • marksman
  • mqttmultimeter
  • msbuild
  • nbxplorer
  • netcoredbg
  • networkminer
  • omnisharp-roslyn
  • opentabletdriver
  • openutau
  • pablodraw
  • parabolic
  • pbm
  • pinta
  • pupdate
  • retrospy
  • roslyn
  • roslyn-ls
  • ryujinx
  • scarab
  • slskd
  • technitium-dns-server
  • tone
  • torrentstream

@tie tie force-pushed the dotnet-cross branch 3 times, most recently from 6fc0934 to 5cfab6d Compare May 20, 2024 03:57
@tie tie added the 8.has: tests This PR has tests label May 20, 2024
This change refactors internal hooks used by buildDotnetModule to
support derivations with structured attributes. Note that this changes
variable names that the internal hooks expect.
@superherointj superherointj added the 6.topic: dotnet Language: .NET label Jun 4, 2024
@corngood
Copy link
Contributor

This all looks okay to me, but it's a big change. Anyone else in @NixOS/dotnet want to review?

@corngood corngood self-requested a review June 12, 2024 17:14
@corngood corngood dismissed their stale review June 12, 2024 17:15

obsolete

@siraben
Copy link
Member

siraben commented Jun 16, 2024

Result of nixpkgs-review pr 313005 run on x86_64-linux 1

80 packages built:
  • ArchiSteamFarm
  • BeatSaberModManager
  • alttpr-opentracker
  • am2rlauncher
  • audiobookshelf
  • avalonia-ilspy
  • azure-functions-core-tools
  • bicep
  • boogie (dotnetPackages.Boogie)
  • btcpayserver
  • btcpayserver-altcoins
  • cavalier
  • celeste64
  • certdump
  • csharp-ls
  • csharpier
  • csharprepl
  • cyclonedx-cli
  • dafny
  • denaro
  • depotdownloader
  • discordchatexporter-cli
  • dotnet-outdated
  • eventstore
  • fable
  • famistudio
  • fantomas
  • formula
  • fsautocomplete
  • galaxy-buds-client
  • garnet
  • git-credential-manager
  • github-runner
  • gitversion
  • ilspycmd
  • inklecate
  • jackett
  • jellyfin
  • kavita
  • knossosnet
  • libation
  • lubelogger
  • lumafly
  • marksman
  • mqttmultimeter
  • msbuild
  • naps2
  • nbxplorer
  • netcoredbg
  • networkminer
  • omnisharp-roslyn
  • openra
  • opentabletdriver
  • openutau
  • osu-lazer
  • pablodraw
  • parabolic
  • pbm
  • pinta
  • ps3-disc-dumper
  • pupdate
  • retrospy
  • roslyn
  • roslyn-ls
  • ryujinx
  • scarab
  • slskd
  • space-station-14-launcher
  • technitium-dns-server
  • tests.dotnet.project-references
  • tests.dotnet.structured-attrs.check-output
  • tests.dotnet.structured-attrs.no-structured-attrs
  • tests.dotnet.use-dotnet-from-env.fallback
  • tests.dotnet.use-dotnet-from-env.use-dotnet-path-env
  • tests.dotnet.use-dotnet-from-env.use-dotnet-root-env
  • tests.dotnet.use-dotnet-from-env.without-fallback
  • tone
  • torrentstream
  • wasabibackend
  • xivlauncher

Copy link
Member

@siraben siraben left a comment

Choose a reason for hiding this comment

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

LGTM, but would like someone who works with dotnet to approve.

Copy link
Contributor

@GGG-KILLER GGG-KILLER left a comment

Choose a reason for hiding this comment

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

Looks good to me as well

@DontEatOreo
Copy link
Member

DontEatOreo commented Jun 17, 2024

Result of nixpkgs-review pr 313005 run on aarch64-darwin 1

1 package marked as broken and skipped:
  • pablodraw
3 packages failed to build:
  • azure-functions-core-tools
  • certdump
  • marksman
36 packages built:
  • ArchiSteamFarm
  • avalonia-ilspy
  • bicep
  • boogie (dotnetPackages.Boogie)
  • btcpayserver
  • btcpayserver-altcoins
  • csharp-ls
  • csharpier
  • csharprepl
  • cyclonedx-cli
  • dafny
  • depotdownloader
  • dotnet-outdated
  • famistudio
  • fantomas
  • formula
  • fsautocomplete
  • garnet
  • git-credential-manager
  • github-runner
  • gitversion
  • ilspycmd
  • jackett
  • jellyfin
  • knossosnet
  • libation
  • lubelogger
  • msbuild
  • nbxplorer
  • netcoredbg
  • omnisharp-roslyn
  • openutau
  • roslyn
  • roslyn-ls
  • tests.dotnet.structured-attrs.no-structured-attrs
  • torrentstream

EDIT:

azure-functions-core-tools failed due to sandbox-exec: pattern serialization length 68938 exceeds maximum (65535)

certdump failed due to error NETSDK1084: There is no application host available for the specified RuntimeIdentifier 'osx-arm64'. [/private/tmp/nix-build-certdump-unstable-2023-07-12.drv-0/source/CertDump/CertDump.csproj]

marksman failed due to:

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
System.Net.Sockets.SocketException (13): Permission denied
   at System.Net.Sockets.Socket.DoBind(EndPoint endPointSnapshot, SocketAddress socketAddress)
   at System.Net.Sockets.Socket.Bind(EndPoint localEP)
   at System.Net.Sockets.TcpListener.Start(Int32 backlog)
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.SocketServer.Start(String endPoint) in /_/src/Microsoft.TestPlatform.CommunicationUtilities/SocketServer.cs:line 65
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestSender.InitializeCommunication() in /_/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs:line 168
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, String runSettings) in /_/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs:line 204
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyExecutionManager.InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) in /_/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs:line 182

Test Run Aborted.

@corngood corngood merged commit b544727 into NixOS:master Jun 17, 2024
21 checks passed
@GGG-KILLER
Copy link
Contributor

Result of nixpkgs-review pr 313005 run on aarch64-darwin 1

1 package marked as broken and skipped:
3 packages failed to build:
36 packages built:
EDIT:

azure-functions-core-tools failed due to sandbox-exec: pattern serialization length 68938 exceeds maximum (65535)

certdump failed due to error NETSDK1084: There is no application host available for the specified RuntimeIdentifier 'osx-arm64'. [/private/tmp/nix-build-certdump-unstable-2023-07-12.drv-0/source/CertDump/CertDump.csproj]

marksman failed due to:

Starting test execution, please wait...
A total of 1 test files matched the specified pattern.
System.Net.Sockets.SocketException (13): Permission denied
   at System.Net.Sockets.Socket.DoBind(EndPoint endPointSnapshot, SocketAddress socketAddress)
   at System.Net.Sockets.Socket.Bind(EndPoint localEP)
   at System.Net.Sockets.TcpListener.Start(Int32 backlog)
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.SocketServer.Start(String endPoint) in /_/src/Microsoft.TestPlatform.CommunicationUtilities/SocketServer.cs:line 65
   at Microsoft.VisualStudio.TestPlatform.CommunicationUtilities.TestRequestSender.InitializeCommunication() in /_/src/Microsoft.TestPlatform.CommunicationUtilities/TestRequestSender.cs:line 168
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyOperationManager.SetupChannel(IEnumerable`1 sources, String runSettings) in /_/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyOperationManager.cs:line 204
   at Microsoft.VisualStudio.TestPlatform.CrossPlatEngine.Client.ProxyExecutionManager.InitializeTestRun(TestRunCriteria testRunCriteria, IInternalTestRunEventsHandler eventHandler) in /_/src/Microsoft.TestPlatform.CrossPlatEngine/Client/ProxyExecutionManager.cs:line 182

Test Run Aborted.

The error in azure-functions-core-tools seems like the only one that might be caused by this PR, if you could try to build these using master without this PR it'd be nice, just to know if this PR caused it or not

@corngood
Copy link
Contributor

Sorry, I merged this because I thought all those failures were pre-existing, but you might be right about azure-functions-core-tools.

@corngood
Copy link
Contributor

Actually I just tested azure-functions-core-tools without this change, and it fails with an even longer sandbox profile, so I don't think it's a regression.

@DontEatOreo
Copy link
Member

I was able to build azure-functions-core-tools on rev 683aa7c with nix-build -A but couldn't when using nixpkgs-review

nix run nixpkgs#nixpkgs-review -- rev HEAD -p azure-functions-core-tool

$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0
remote: Enumerating objects: 195, done.
remote: Counting objects: 100% (131/131), done.
remote: Compressing objects: 100% (54/54), done.
remote: Total 71 (delta 43), reused 41 (delta 14), pack-reused 0
Unpacking objects: 100% (71/71), 14.87 KiB | 80.00 KiB/s, done.
From https://github.com/NixOS/nixpkgs
   219bc27bcfea..2e2f30243165  master     -> refs/nixpkgs-review/0
$ git worktree add /Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs 2e2f30243165b1acd638ef09bf79430b7aad802a
Preparing worktree (detached HEAD 2e2f30243165)
Updating files: 100% (41698/41698), done.
HEAD is now at 2e2f30243165 Merge pull request #320521 from GaetanLepage/insightface
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f <nixpkgs> --nix-path nixpkgs=/Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs nixpkgs-overlays=/tmp/tmpcim2jiwp -qaP --xml --out-path --show-trace --no-allow-import-from-derivation
$ git merge --no-commit --no-ff 683aa7c4e385509ca651d49eeb35e58c7a1baad6
Already up to date.
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f <nixpkgs> --nix-path nixpkgs=/Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs nixpkgs-overlays=/tmp/tmpcim2jiwp -qaP --xml --out-path --show-trace --no-allow-import-from-derivation --meta
These packages do not exist:
azure-functions-core-tool
$ git worktree remove -f /Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs

@tie
Copy link
Member Author

tie commented Jun 17, 2024

W.r.t. azure-functions-core-tools, is the failure specific to the sandboxed darwin builds? FWIW builds from cache.nixos.org do not use sandbox for Darwin builders, and nix-build -A does substitute those even if sandbox = true in local Nix configuration.

@tie
Copy link
Member Author

tie commented Jun 17, 2024

@DontEatOreo, could it be that Nix substituted the derivation output from cache.nixos.org for you? I can’t build azure-functions-core-tools on 683aa7c with sandbox enabled on aarch64-darwin.

Did you pass --check flag to nix-build to actually build the derivation? I.e.

nix-build --check -A azure-functions-core-tool

@corngood
Copy link
Contributor

I just tested on master, and it's broken in the sandbox, but builds with --no-sandbox.

@DontEatOreo in your example above you were using azure-functions-core-tool without the 's'.

I think it's just: NixOS/nix#4119

@DontEatOreo
Copy link
Member

@DontEatOreo, could it be that Nix substituted the derivation output from cache.nixos.org for you? I can’t build azure-functions-core-tools on 683aa7c with sandbox enabled on aarch64-darwin.

Did you pass --check flag to nix-build to actually build the derivation? I.e.

nix-build --check -A azure-functions-core-tool

I didn't, but after running nix-build --check -A azure-functions-core-tools, I got the sandbox-exec error even on commit 683aa7c (I don't have sandbox set to anything either).

I just tested on master, and it's broken in the sandbox, but builds with --no-sandbox.

@DontEatOreo in your example above you were using azure-functions-core-tool without the 's'.

I think it's just: NixOS/nix#4119

Oh, right. My bad. Sorry about that! 😅

Running the command again with proper package name still fails building:

nix run nixpkgs#nixpkgs-review -- rev HEAD -p azure-functions-core-tools

$ git -c fetch.prune=false fetch --no-tags --force https://github.com/NixOS/nixpkgs master:refs/nixpkgs-review/0
remote: Enumerating objects: 375, done.
remote: Counting objects: 100% (341/341), done.
remote: Compressing objects: 100% (180/180), done.
remote: Total 375 (delta 219), reused 235 (delta 150), pack-reused 34
Receiving objects: 100% (375/375), 992.53 KiB | 5.06 MiB/s, done.
Resolving deltas: 100% (225/225), completed with 54 local objects.
From https://github.com/NixOS/nixpkgs
   be79f03572ad..36bd9a0872ad  master     -> refs/nixpkgs-review/0
$ git worktree add /Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs 36bd9a0872ad6fbe2312f6f4b67902cc66739dd3
Preparing worktree (detached HEAD 36bd9a0872ad)
Updating files: 100% (41700/41700), done.
HEAD is now at 36bd9a0872ad Merge pull request #315140 from alexfmpe/ispc-1.24
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f <nixpkgs> --nix-path nixpkgs=/Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs nixpkgs-overlays=/tmp/tmpfvgw0_uq -qaP --xml --out-path --show-trace --no-allow-import-from-derivation
$ git merge --no-commit --no-ff 683aa7c4e385509ca651d49eeb35e58c7a1baad6
Already up to date.
$ nix-env --extra-experimental-features no-url-literals --option system aarch64-darwin -f <nixpkgs> --nix-path nixpkgs=/Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs nixpkgs-overlays=/tmp/tmpfvgw0_uq -qaP --xml --out-path --show-trace --no-allow-import-from-derivation --meta
The following packages specified with `-p` are not rebuilt by the pull request
azure-functions-core-tools
$ git worktree remove -f /Users/DontEatOreo/.cache/nixpkgs-review/rev-683aa7c4e385509ca651d49eeb35e58c7a1baad6/nixpkgs

K900 added a commit to K900/nixpkgs that referenced this pull request Jun 18, 2024
@K900
Copy link
Contributor

K900 commented Jun 18, 2024

This regressed Jellyfin, fix is #320711

K900 added a commit that referenced this pull request Jun 18, 2024
jellyfin: fix makeWrapperArgs after #313005
@tie
Copy link
Member Author

tie commented Jun 18, 2024

@K900, I’m sorry I missed this regression, I’ve created a PR with a fix. Apparently nixpkgs-review and ofborg didn’t run passthru.tests and nixosTests 🧐

@corngood
Copy link
Contributor

Apparently nixpkgs-review and ofborg didn’t run passthru.tests and nixosTests 🧐

@tie Yeah, that's annoying. You might want to track this: Mic92/nixpkgs-review#77

In the meantime I've been running this from the nixpkgs-review shell:

nix-build --expr 'map (a: if a?tests then a.tests else []) ((import ./attrs.nix) (import ./nixpkgs {}))'

It runs the tests from changed packages, but not all changed tests.

github-actions bot pushed a commit that referenced this pull request Jun 19, 2024
wegank pushed a commit to RaitoBezarius/nixpkgs that referenced this pull request Jun 20, 2024
JohnRTitor added a commit that referenced this pull request Jun 23, 2024
[Backport release-24.05] jellyfin: fix makeWrapperArgs after #313005
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cross-compilation Building packages on a different platform than they will be used on 6.topic: dotnet Language: .NET 8.has: package (new) This PR adds a new package 8.has: tests This PR has tests 10.rebuild-darwin: 11-100 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants