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

dotnet 3.1.108 (new formula) #60929

Closed
wants to merge 1 commit into from
Closed

Conversation

asbjornu
Copy link
Contributor

  • Have you followed the guidelines for contributing?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing brew install <formula>)?

Thanks to all the valuable feedback given in #53092 and with dotnet/source-build#1685 resolved, dotnet finally builds successfully on macOS and this formula should be ready to go.

Supersedes #3691 and fixes dotnet/sdk#4600.

Copy link
Member

@SMillerDev SMillerDev left a comment

Choose a reason for hiding this comment

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

Thanks for the work here, mostly some style issues to go

Formula/dotnet.rb Outdated Show resolved Hide resolved
Formula/dotnet.rb Outdated Show resolved Hide resolved
Formula/dotnet.rb Outdated Show resolved Hide resolved
Formula/dotnet.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

Looks like a missing header (maybe missing dependency?): https://github.com/Homebrew/homebrew-core/pull/60929/checks?check_run_id=1097197052#step:5:3938

@asbjornu asbjornu force-pushed the feature/dotnet branch 2 times, most recently from daa2d98 to ce3969a Compare September 10, 2020 21:03
@asbjornu
Copy link
Contributor Author

asbjornu commented Sep 10, 2020

I believe I have fixed all style problems now, but as @MikeMcQuaid writes, the build fails.

10.14 fails with the following error:

==> brew audit dotnet --online --new-formula
==> FAILED
Error: 3 problems in 1 formula detected
dotnet:
  * Non-libraries were installed to "/usr/local/opt/dotnet/lib"
    Installing non-libraries to "lib" is discouraged.
    The offending files are:
      /usr/local/opt/dotnet/lib/ThirdPartyNotices.txt
            /usr/local/opt/dotnet/lib/dotnet
            /usr/local/opt/dotnet/lib/LICENSE.txt
  * Dependency 'curl' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.
  * Dependency 'icu4c' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

As mentioned, I've tried to declare curl and icu4c with uses_from_macos, but that only causes the build to fail for me locally. The build is successful as the formula is written right now on my iMac with 10.14.6.

Also, I'm not sure how to best fix the Installing non-libraries to "lib" is discouraged problem.

Then there's this, which I need help to decode:

==> brew linkage --test dotnet
==> FAILED
Broken dependencies:
  /usr/local/opt/curl/lib/libcurl.4.dylib (curl)

10.15 fails with the following error, which I suppose I have to take to the .NET team. I do expect them to ask for the contents of /private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/logs/coreclr.log, though. Is that possible to get our hands on in any way?

  'coreclr' failed during build.
  See '/private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/logs/coreclr.log' for more information.
  'corefx' failed during build.
  'msbuild' failed during build.
  'known-good' failed during build.

Build FAILED.

/private/tmp/dotnet-20200910-8463-nvg0yf/packages/restored/microsoft.dotnet.arcade.sdk/1.0.0-beta.19359.6/tools/ProjectLayout.props(8,3): warning MSB4011: "/private/tmp/dotnet-20200910-8463-nvg0yf/packages/restored/microsoft.dotnet.arcade.sdk/1.0.0-beta.19359.6/tools/RepoLayout.props" cannot be imported again. It was already imported at "/private/tmp/dotnet-20200910-8463-nvg0yf/packages/restored/microsoft.dotnet.arcade.sdk/1.0.0-beta.19359.6/tools/Build.proj (52,3)". This is most likely a build authoring error. This subsequent import will be ignored. 
/private/tmp/dotnet-20200910-8463-nvg0yf/repos/Directory.Build.targets(296,5): error MSB3073: The command "/private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/src/coreclr.f897710bc4efe6a046068fde0acf641667a8fff5//build.sh x64 Release skiptests -ignoreWarnings -skipmanagedtools -skiprestore -nopgooptimize msbuildonunsupportedplatform cmakeargs -DCLR_CMAKE_USE_SYSTEM_LIBUNWIND=TRUE /bl /p:ILLinkTrimAssembly=false /p:CheckCDefines=false /p:PackagesDir=/private/tmp/dotnet-20200910-8463-nvg0yf/packages/restored/ /p:ContinuousIntegrationBuild=true /p:VersionSuffix="servicing" -warnAsError:0 /v:minimal  /p:DotNetPackageVersionPropsPath=/private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/obj/x64/Release/PackageVersions.props /p:DotNetRestoreSourcePropsPath=/private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/obj/x64/Release/RestoreSources.props >> /private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/logs/coreclr.log 2>&1" exited with code 1. [/private/tmp/dotnet-20200910-8463-nvg0yf/repos/coreclr.proj]

Copy link
Contributor

@SeekingMeaning SeekingMeaning left a comment

Choose a reason for hiding this comment

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

Nice work!

Formula/dotnet.rb Outdated Show resolved Hide resolved
Formula/dotnet.rb Outdated Show resolved Hide resolved
@MikeMcQuaid
Copy link
Member

        /usr/local/opt/dotnet/lib/dotnet

What is this file?

  /usr/local/opt/dotnet/lib/ThirdPartyNotices.txt
        /usr/local/opt/dotnet/lib/LICENSE.txt

These should be moved to doc.

As mentioned, I've tried to declare curl and icu4c with uses_from_macos, but that only causes the build to fail for me locally. The build is successful as the formula is written right now on my iMac with 10.14.6.

Thanks for checking 👍🏻

Broken dependencies:
  /usr/local/opt/curl/lib/libcurl.4.dylib (curl)

That means this isn't a :build dependency but is needed at runtime too.

10.15 fails with the following error, which I suppose I have to take to the .NET team. I do expect them to ask for the contents of /private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/logs/coreclr.log, though. Is that possible to get our hands on in any way?

On the GitHub Actions workers: I don't think so, really. Someone on 10.15 (e.g. me) could do it locally for you and upload it?

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Feedback left as issue comments.

@asbjornu
Copy link
Contributor Author

        /usr/local/opt/dotnet/lib/dotnet

What is this file?

It's the binary executable. Extracting all files from the build output tarball into libexec instead of lib as suggested by @SeekingMeaning seems to have fixed these auditorial problems.

  /usr/local/opt/dotnet/lib/ThirdPartyNotices.txt
        /usr/local/opt/dotnet/lib/LICENSE.txt

These should be moved to doc.

brew audit has stopped complaining now that they reside in libexec instead. Should I still move them to doc?

As mentioned, I've tried to declare curl and icu4c with uses_from_macos, but that only causes the build to fail for me locally. The build is successful as the formula is written right now on my iMac with 10.14.6.

Thanks for checking 👍🏻

Is it possible to circumvent the complaints from brew audit regarding its insistence on replacing depends_on with uses_from_macos?

Broken dependencies:
  /usr/local/opt/curl/lib/libcurl.4.dylib (curl)

That means this isn't a :build dependency but is needed at runtime too.

Indeed, I've made that change now with @SeekingMeaning's code review suggestion.

10.15 fails with the following error, which I suppose I have to take to the .NET team. I do expect them to ask for the contents of /private/tmp/dotnet-20200910-8463-nvg0yf/artifacts/logs/coreclr.log, though. Is that possible to get our hands on in any way?

On the GitHub Actions workers: I don't think so, really. Someone on 10.15 (e.g. me) could do it locally for you and upload it?

Since the failure seems consistent for 10.15, I would really appreciate if you coud! 🙏

@MikeMcQuaid
Copy link
Member

brew audit has stopped complaining now that they reside in libexec instead. Should I still move them to doc?

Yes please!

Is it possible to circumvent the complaints from brew audit regarding its insistence on replacing depends_on with uses_from_macos?

It's ok, you can ignore it and it'll go away after this is merged.

Since the failure seems consistent for 10.15, I would really appreciate if you coud! 🙏

Let me know exactly what you'd like me to run and what files to upload where and I will!

@asbjornu asbjornu force-pushed the feature/dotnet branch 2 times, most recently from 282c49c to 1c852da Compare September 11, 2020 22:31
@asbjornu
Copy link
Contributor Author

asbjornu commented Sep 11, 2020

brew audit has stopped complaining now that they reside in libexec instead. Should I still move them to doc?

Yes please!

👍

Let me know exactly what you'd like me to run and what files to upload where and I will!

If you check out this PR and run brew install --keep-tmp --verbose --debug dotnet and then upload the coreclr.log of which path is written to the console when the build fails, that would be very helpful!

Formula/dotnet.rb Outdated Show resolved Hide resolved
@SMillerDev
Copy link
Member

SMillerDev commented Sep 12, 2020

If you check out this PR and run brew install --keep-tmp --verbose --debug dotnet and then upload the coreclr.log of which path is written to the console when the build fails, that would be very helpful!

Running now, will edit with the result.

EDIT: I suspect it's this, but I attached the log
coreclr.log

/tmp/dotnet-20200912-76027-168kcki/artifacts/src/coreclr.f897710bc4efe6a046068fde0acf641667a8fff5/src/pal/src/exception/remote-unwind.cpp:68:10: fatal error: 'elf.h' file not found
#include <elf.h>
         ^~~~~~~

@asbjornu
Copy link
Contributor Author

Thanks, @SMillerDev! Does this mean I should add a dependency on libelf? Is that possible to do only for 10.15, as it doesn't seem to be required on 10.14 or 10.13? Also, this SO question indicates that libelf does not actually add elf.h, so I might need to source that from Apple's dtrace source instead? Thoughts?

@SMillerDev
Copy link
Member

I'm not sure how feasible either solution would be from a packaging standpoint. It might be better to report the issue upstream and see if they have a solution.

@asbjornu
Copy link
Contributor Author

Ideas on how we can proceed with the suggestions made in dotnet/source-build#1744 (comment) is welcome. As I don't have 10.15 and Microsoft themselves are unable to reproduce the problem, it would be incredible if @SMillerDev or someone else with 10.15 who are able to reproduce this can assist with the necessary debugging to figure out the source of this issue. 🙏

@asbjornu
Copy link
Contributor Author

asbjornu commented Oct 9, 2020

The PR came in too late for this, it won't be in v3.1.109-SDK unfortunately. It'll be in v3.1.110-SDK next month.

@dagood, I see. Thanks for elaborating.

If you can create a patch file and copy it into the repo, that's probably a little clearer than this nested patch within a patch, but I'm not sure what's possible in the homebrew infra.

It seems like Gist is used with resource in Homebrew already:

resource "secret.gpg" do
url "https://gist.github.com/bfontaine/5b0e3efa97e2dc42a970/raw/915e802578339ddde2967de541ed65cb76cd14b9/secret.gpg"
sha256 "eec8f32a401d1077feb19ea4b8e1816feeac02b9bfe6bd09e75c9985ff740890"
end

So I suppose that with it we could create the file patches/coreclr/unw_flags.patch from your Gist and revert back to the 3.1.108-SDK tag? I tried the following, but it didn't work:

resource "patches/coreclr/0001-Fix-elf.h-file-not-found-on-macOS-10.15-1789.patch" do
    url "https://gist.github.com/dagood/0ad394e4f73fd9f80ae85bc90de90296/raw/02b14212f024d56f477d1df85aac7997ddffcf1d/0001-Fix-elf.h-file-not-found-on-macOS-10.15-1789.patch"
    sha256 "dedd27c5722fbd69afeaaef28c3f4e4299855be4842028cd74e0ce8c0156ab77"
end

@omajid
Copy link

omajid commented Oct 10, 2020

    url "https://gist.github.com/dagood/0ad394e4f73fd9f80ae85bc90de90296/raw/02b14212f024d56f477d1df85aac7997ddffcf1d/0001-Fix-elf.h-file-not-found-on-macOS-10.15-1789.patch"

That's a patch of a patch. If you are placing this file under patches/coreclr/, you should probably use https://raw.githubusercontent.com/dotnet/source-build/release/3.1/patches/coreclr/0005-Fix-bad-configure-tests.patch directly.

@asbjornu
Copy link
Contributor Author

Thanks, @omajid. But it seems like I'm unable to use resource since it doesn't allow me to write the file to the buildpath (the variable isn't initialized before install) and patch also won't work since it requires the source to be patched to be available before install.

So unless I go about manually downloading the patch in install just before executing build.sh, I don't see how I can get the patch downloaded to the correct place at the correct time. I'm not sure such a hack would be accepted and it feels like overdoing it when we can just wait for 3.1.110 to become available.

@dagood
Copy link

dagood commented Oct 11, 2020

But it seems like I'm unable to use resource since it doesn't allow me to write the file to the buildpath (the variable isn't initialized before install)

Doesn't the formula control how install works? Can't you make it copy the resource to the source directory before running ./build.sh?

patch also won't work since it requires the source to be patched to be available before install.

Why isn't the source available before install? It looks like it's normally fine:

url "https://deb.debian.org/debian/pool/main/f/fakeroot/fakeroot_1.24.orig.tar.gz"

patch do
url "https://bugs.debian.org/cgi-bin/bugreport.cgi?msg=5;filename=0002-OS-X-10.10-introduced-id_t-int-in-gs-etpriority.patch;att=2;bug=766649"
sha256 "e0823a8cfe9f4549eb4f0385a9cd611247c3a11c0452b5f80ea6122af4854b7c"
end

@asbjornu
Copy link
Contributor Author

Doesn't the formula control how install works?

To some extent, yes. It is, of course, "just Ruby", so I can do whatever I want, but I don't think doing whatever I want is going to get this PR accepted.

Can't you make it copy the resource to the source directory before running ./build.sh?

Indeed, I can. Thanks for the idea, I just found out I can do this:

resource("0005-Fix-bad-configure-tests.patch").stage buildpath/"patches/coreclr/0005-Fix-bad-configure-tests.patch"

I'm running a build with this in it just now, so fingers crossed this will work.

patch also won't work since it requires the source to be patched to be available before install.

Why isn't the source available before install?

The url is being cloned just before install is invoked, so everything outside install such as resource and patch does not have access to the source or buildpath.

@asbjornu
Copy link
Contributor Author

@dagood, @omajid, even with patches/coreclr/0005-Fix-bad-configure-tests.patch in place, the build fails with fatal error: 'elf.h' file not found. It's almost like the patch isn't picked up during the build at all. How do I go about debugging this?

@dagood
Copy link

dagood commented Oct 12, 2020

Can you look at the source to see if the patch was applied? Seems like a simple check.

For more details (patch command output) you can also look at and share the binlogs (https://aka.ms/binlog) to see what happened. find . -iname '*.binlog'--the one in artifacts/log/*/ would show the patch being applied.

Formula/dotnet.rb Outdated Show resolved Hide resolved
@asbjornu
Copy link
Contributor Author

Can you look at the source to see if the patch was applied? Seems like a simple check.

Seems like ./artifacts/src/coreclr.f897710bc4efe6a046068fde0acf641667a8fff5/src/pal/src/configure.cmake is unpatched as line 1033 still says:

set(CMAKE_REQUIRED_FLAGS "-c -Werror=implicit-function-declaration")

For more details (patch command output) you can also look at and share the binlogs (https://aka.ms/binlog) to see what happened. find . -iname '*.binlog'--the one in artifacts/log/*/ would show the patch being applied.

Please have a look at Build_1013014149.binlog.

Formula/dotnet.rb Outdated Show resolved Hide resolved
@asbjornu
Copy link
Contributor Author

There we go, @MikeMcQuaid. The only failures now on all versions of macOS are:

  * Dependency 'curl' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.
  * Dependency 'icu4c' is provided by macOS; please replace 'depends_on' with 'uses_from_macos'.

@asbjornu asbjornu changed the title dotnet 3.1.109 (new formula) dotnet 3.1.108 (new formula) Oct 13, 2020
Copy link
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

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

Thanks so much for your contribution and incredible patience! Without people like you submitting PRs we couldn't run this project. You rock, @asbjornu!

@BrewTestBot
Copy link
Member

:shipit: @MikeMcQuaid has triggered a merge.

@asbjornu asbjornu deleted the feature/dotnet branch October 13, 2020 14:09
@asbjornu
Copy link
Contributor Author

Thanks for all the assistance to everyone involved, this was a team effort! ❤️

@MikeMcQuaid
Copy link
Member

You're very welcome @asbjornu! CC me on the relevant GitVersion PR; I'm interested to see what it looks like after all this!

@SMillerDev
Copy link
Member

Thanks @asbjornu ! Without contributions like yours it'd be impossible to keep homebrew going with the high standards that users have come to expect from the project. You can feel good knowing that you've made the world a bit better for homebrew and.NET users around the world! 👍 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new formula PR adds a new formula to Homebrew/homebrew-core
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Homebrew formula
10 participants