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

azure-functions-core-tools: 4.0.5455 -> 4.0.6594 #361077

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

Detegr
Copy link
Contributor

@Detegr Detegr commented Dec 2, 2024

Closes #360154

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.

@Detegr
Copy link
Contributor Author

Detegr commented Dec 2, 2024

I'm getting the following error locally:

       > Patchelfing microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg as microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg.47.nix-patched
       > patchelf: patchelf.cc:1095: void setSubstr(std::string&, unsigned int, const std::string&): Assertion `pos + t.size() <= s.size()' failed.

Made the pull request to make it reproducible and/or to see if it happens in the CI as well.

@gepbird gepbird mentioned this pull request Dec 2, 2024
17 tasks
@gepbird
Copy link
Contributor

gepbird commented Dec 2, 2024

I could reproduce that error:

       > Patching package microsoft.azure.functions.dotnetisolatednativehost/1.0.11
       > Patchelfing microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost as microsoft.azure.functions.dotnetisolatednativehost/1.0.11/co
ntentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.47.nix-patched
       > Patchelfing microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg as microsoft.azure.functions.dotnetisolatednativehost/1.0.1
1/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg.47.nix-patched
       > patchelf: patchelf.cc:1095: void setSubstr(std::string&, unsigned int, const std::string&): Assertion `pos + t.size() <= s.size()' failed.
       > /nix/store/w9yd8z7rxbnafzkkpmh9mznkql1cp45d-patch-nupkgs/bin/patch-nupkgs: line 14:    60 Aborted                 (core dumped) /nix/store/xjlvb8x7z3f5rxxswa3c6ghay74a58jm-patchelf-0.15.0/bin/patchelf --add-needed libicui18n.so --add-needed libicuuc.so --add-needed libz.so --add-needed libssl.so "$tmp"

My first instict was adding those libraries to buildInputs but that doesn't seem to help:

  buildInputs = [
    openssl
    zlib
    icu
  ];

@gepbird
Copy link
Contributor

gepbird commented Dec 3, 2024

@NixOS/dotnet any idea about the aforementioned error?

@GGG-KILLER
Copy link
Contributor

It seems like an issue with the way we detect what can be patchelf'd or not. I'll try to make a fix for it.

@GGG-KILLER
Copy link
Contributor

@Detegr or @gepbird could either of you check if the changes in #361450 make it build properly for you now?

@gepbird
Copy link
Contributor

gepbird commented Dec 4, 2024

@Detegr or @gepbird could either of you check if the changes in #361450 make it build properly for you now?

@GGG-KILLER I cherry picked the commit from your PR on top of this and unfortunately the build failed.

Build log
       > Running phase: fixupPhase                                                                                                                                                                                
       > Patching package microsoft.azure.functions.dotnetisolatednativehost/1.0.11                                                                           
       > patchelf: cannot find section '.interp'. The input file is most likely statically linked                                                             
       > Patchelfing microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost as microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.47.nix-patched
       > Patchelfing microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg as microsoft.azure.functions.dotnetisolatednativehost/1.0.11/contentFiles/any/any/workers/dotnet-isolated/bin/FunctionsNetHost.dbg.47.nix-patched
       > patchelf: patchelf.cc:1095: void setSubstr(std::string&, unsigned int, const std::string&): Assertion `pos + t.size() <= s.size()' failed.                                                                                                                                                                         
       > /nix/store/ng4s9mlk4phhnz4d59c14srfhrsv1cnz-patch-nupkgs/bin/patch-nupkgs: line 17:    72 Aborted                 (core dumped) /nix/store/xjlvb8x7z3f5rxxswa3c6ghay74a58jm-patchelf-0.15.0/bin/patchelf --add-needed libicui18n.so --add-needed libicuuc.so --add-needed libz.so --add-needed libssl.so "$tmp"

(going to sleep now)

@GGG-KILLER
Copy link
Contributor

GGG-KILLER commented Dec 5, 2024

Ok, got it to compile with the latest changes in #361450, but now there's an error on the buildDotnetModule usage:

@nix { "action": "setPhase", "phase": "installPhase" }
Executing dotnetInstallHook
/nix/store/wlknwwx8f79lq8n07b0xh7ddjrrk191h-dotnet-sdk-8.0.404/share/dotnet/sdk/8.0.404/Current/SolutionFile/ImportAfter/Microsoft.NET.Sdk.Solution.targets(36,5): warning NETSDK1194: The "--output" option isn't supported when building a solution. Specifying a solution-level output path results in all projects copying outputs to the same directory, which can lead to inconsistent builds. [/build/source/Azure.Functions.Cli.sln]
/nix/store/wlknwwx8f79lq8n07b0xh7ddjrrk191h-dotnet-sdk-8.0.404/share/dotnet/sdk/8.0.404/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.CrossTargeting.targets(31,5): error NETSDK1129: The 'Publish' target is not supported without specifying a target framework. The current project targets multiple frameworks, you must specify one of the following frameworks in order to publish: net8.0 [/build/source/test/Azure.Functions.Cli.Tests/Azure.Functions.Cli.Tests.csproj]
/nix/store/wlknwwx8f79lq8n07b0xh7ddjrrk191h-dotnet-sdk-8.0.404/share/dotnet/sdk/8.0.404/Sdks/Microsoft.NET.Sdk/targets/Microsoft.NET.Sdk.CrossTargeting.targets(31,5): error NETSDK1129: The 'Publish' target is not supported without specifying a target framework. The current project targets multiple frameworks, you must specify one of the following frameworks in order to publish: net8.0 [/build/source/src/Azure.Functions.Cli/Azure.Functions.Cli.csproj]

Adding the following line fixed it:

  dotnetFlags = [ "-p:TargetFramework=net8.0" ];

@GGG-KILLER
Copy link
Contributor

PR fixing the bug you were encountering has been merged, if you could rebase and check it it works.

@Detegr
Copy link
Contributor Author

Detegr commented Dec 10, 2024

Updated the PR with dotnetFlags set. Seems to be working now, thanks!

@gepbird gepbird self-requested a review December 10, 2024 10:47
@corngood
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 361077


x86_64-linux

✅ 1 package built:
  • azure-functions-core-tools

@corngood corngood added the backport release-24.11 Backport PR automatically label Dec 10, 2024
Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Thanks, your changes LGTM and the package works with .NET 8!

A few improvements and nits, but feel free to ignore if you think it's too unrealted:

  • run nixfmt on the package.nix
  • drop --replace as it is deprecated
-      --replace "CheckExitCode(\"/bin/bash" "CheckExitCode(\"${stdenv.shell}"
+      --replace-fail "CheckExitCode(\"/bin/bash" "CheckExitCode(\"${stdenv.shell}"
  • remove with lib; from meta
  • version checking
+  doInstallCheck = true;
+  nativeInstallCheckInputs = [
+    versionCheckHook
+  ];
  • set meta.mainProgram
  • add an update script
  • remove rec next to buildDotnetModule

src = fetchFromGitHub {
owner = "Azure";
repo = "azure-functions-core-tools";
rev = version;
sha256 = "sha256-Ip1m0/l0YWFosYfp8UeREg9DP5pnvRnXyAaAuch7Op4=";
sha256 = "sha256-tUNiyvIjaIrdo6377IdXND7YgIk9zKkazDHV4kiWYa8=";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sha256 = "sha256-tUNiyvIjaIrdo6377IdXND7YgIk9zKkazDHV4kiWYa8=";
hash = "sha256-tUNiyvIjaIrdo6377IdXND7YgIk9zKkazDHV4kiWYa8=";

@@ -6,12 +6,12 @@
dotnetCorePackages,
}:
let
version = "4.0.5455";
version = "4.0.6594";
src = fetchFromGitHub {
owner = "Azure";
repo = "azure-functions-core-tools";
rev = version;
Copy link
Contributor

Choose a reason for hiding this comment

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

See #355973

Suggested change
rev = version;
tag = version;

@GGG-KILLER
Copy link
Contributor

GGG-KILLER commented Dec 10, 2024

I wouldn't ignore the nixfmt point nor the --replace-fail one. They're both important.

The others would be tolerable if not done (but should be done ideally), however without meta.mainProgram, lib.getExe and nix run may not work properly. But none of them are unrelated overall IMHO.

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two reputable people label Dec 10, 2024
@ofborg ofborg bot requested a review from mdarocha December 11, 2024 04:50
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1 10.rebuild-linux: 1 labels Dec 11, 2024
@corngood
Copy link
Contributor

I wouldn't ignore the nixfmt point nor the --replace-fail one. They're both important.

The others would be tolerable if not done (but should be done ideally), however without meta.mainProgram, lib.getExe and nix run may not work properly. But none of them are unrelated overall IMHO.

This is all good stuff, but this is essentially a simple update PR, and I don't think it makes sense to block it.

@Detegr
Copy link
Contributor Author

Detegr commented Dec 11, 2024

Fixed review comments. I had to left out version check as the software seems to report 4.0.1 for every 4.0.xxxx version 🤷

@corngood
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 361077


x86_64-linux

✅ 1 package built:
  • azure-functions-core-tools

stdenv,
fetchFromGitHub,
buildDotnetModule,
buildGoModule,
dotnetCorePackages,
versionCheckHook,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused, but its not worth blocking on this. Unfortunate that upstream doesn't properly update their version

Suggested change
versionCheckHook,

@corngood corngood merged commit d98ac94 into NixOS:master Dec 13, 2024
38 of 39 checks passed
@nixpkgs-ci
Copy link
Contributor

nixpkgs-ci bot commented Dec 13, 2024

Successfully created backport PR for release-24.11:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes 12.approvals: 2 This PR was reviewed and approved by two reputable people backport release-24.11 Backport PR automatically
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Build failure: azure-functions-core-tools
5 participants