Skip to content

WIP: Initial dotnet formula#3691

Closed
haf wants to merge 1 commit into
Homebrew:masterfrom
haf:feature/dotnet
Closed

WIP: Initial dotnet formula#3691
haf wants to merge 1 commit into
Homebrew:masterfrom
haf:feature/dotnet

Conversation

@haf

@haf haf commented Aug 7, 2016

Copy link
Copy Markdown
Contributor
  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same formula update/change?
  • Have you built your formula locally prior to submission with brew install <formula> (where <formula> is the name of the formula you're submitting)?
  • Does your submission pass brew audit --strict --online <formula> (after doing brew install <formula>)?

Ref https://github.com/dotnet/cli/issues/533

Unfortunately MSFT is a bit new on the OSS scene so they release 1.0 RTM tagged as preview2 on github, which is why this formula is not in fact a pre-release formula.

The original pkg file installs like this:

screen shot 2016-08-08 at 00 13 54

In /usr/local/share/dotnet/ that is. TODO: not sure how to copy the stage2 folder to this location, or the Cellar location of the formula.

@haf

haf commented Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Currently failing to build unless the latest openssl binaries are used with brew link --force openssl with:

Unhandled Exception: System.TypeInitializationException: The type initializer for 'Crypto' threw an exception. ---> System.TypeInitializationException: The type initializer for 'CryptoInitializer' threw an exception. ---> System.DllNotFoundException: Unable to load DLL 'System.Security.Cryptography.Native': The specified module could not be found.
 (Exception from HRESULT: 0x8007007E)
   at Interop.CryptoInitializer.EnsureOpenSslInitialized()
   at Interop.CryptoInitializer..cctor()
   --- End of inner exception stack trace ---
   at Interop.Crypto..cctor()
   --- End of inner exception stack trace ---
   at Interop.Crypto.GetRandomBytes(Byte* buf, Int32 num)
   at System.IO.Path.GetCryptoRandomBytes(Byte* bytes, Int32 byteCount)
   at System.IO.Path.GetRandomFileName()
   at Microsoft.DotNet.InternalAbstractions.TemporaryDirectory..ctor()
   at Microsoft.Extensions.EnvironmentAbstractions.DirectoryWrapper.CreateTemporaryDirectory()
   at Microsoft.DotNet.Configurer.NuGetPackagesArchiver..ctor()
   at Microsoft.DotNet.Cli.Program.ConfigureDotNetForFirstTimeUse(INuGetCacheSentinel nugetCacheSentinel)
   at Microsoft.DotNet.Cli.Program.ProcessArgs(String[] args, ITelemetry telemetryClient)
   at Microsoft.DotNet.Cli.Program.Main(String[] args)
/private/tmp/dotnet-20160807-11838-1cf1ilq/cli-1.0.0-preview2/build_projects/dotnet-cli-build/build.sh: line 117: 11982 Abort trap: 6           dotnet restore

Then failing with:

                        Error: Empty installation

Which I suppose is because it places stuff in a special folder. What's the incantation to tell it to fetch all things from stage2, into the cellar folder? (TODO in code)

@haf haf force-pushed the feature/dotnet branch 3 times, most recently from 3220888 to 6a26099 Compare August 7, 2016 22:10
@haf

haf commented Aug 7, 2016

Copy link
Copy Markdown
Contributor Author

Ref https://github.com/dotnet/cli/issues/3964#issuecomment-236832511

Added two TODOs I need help with.

@haf haf force-pushed the feature/dotnet branch from 6a26099 to 2acea54 Compare August 7, 2016 22:16
Comment thread Formula/dotnet.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

this doesn't need to run as root

@haf haf force-pushed the feature/dotnet branch from 2acea54 to f42555b Compare August 8, 2016 07:52
Comment thread Formula/dotnet.rb Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can you please submit this patch to the upstream developers of this project and add a link to the upstream patch submission and explanation of why the patch is needed in a comment in the formula file. Thanks!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's because they require git to build... How do you normally deal with that?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just use a git checkout.

@haf haf force-pushed the feature/dotnet branch 2 times, most recently from b3bd54e to cb137c9 Compare August 10, 2016 07:46
@haf

haf commented Aug 10, 2016

Copy link
Copy Markdown
Contributor Author

@dunn Excellent, thanks for that hint. So then it's only this TODO left before this may even work in some manner: # TODO: how do I tell homebrew that the folders are under stage2?

@dunn

dunn commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

how do I tell homebrew that the folders are under stage2?

Sorry, I'm not sure what you mean. Which folders, and what needs to be done to them?

@haf

haf commented Aug 10, 2016

Copy link
Copy Markdown
Contributor Author

@dunn Normally it would output stuff to prefix, but not this project. Instead it outputs to buildpath/"artifacts/osx.#{MacOS.version}-x64/stage2"; so I want that stuff into the folder (prefix plus something?) where the 'final product' ends up.

@dunn

dunn commented Aug 10, 2016

Copy link
Copy Markdown
Contributor

Compile steps should always leave the build product in the temporary directory buildpath. For projects with configure scripts we pass --prefix=#{prefix} so that make install knows where to move things to; our std_cmake_args does something similar for CMake projects.

TL;DR nothing is ever automatically installed into prefix, so you're doing the right thing by doing it manually.

@haf

haf commented Aug 10, 2016

Copy link
Copy Markdown
Contributor Author

@dunn is this what you're thinking of?

Comment thread Formula/dotnet.rb Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might need to be prefix.install Dir["#{buildpath}/artifacts/osx.#{MacOS.version}-x64/stage2/*"], but if everything other than the dotnet binary belongs in the root prefix (as opposed to lib, libexec, etc.) then yeah, this is the right approach.

@haf

haf commented Aug 11, 2016

Copy link
Copy Markdown
Contributor Author

The current state, when moving dotnet to bin and then using it, will throw this exception:

dotnet
Failed to load , error: dlopen(, 1): no suitable image found.  Did find:
    /usr/local/lib/: not a file
    /usr/lib/: not a file
    /usr/local/lib/: not a file
    /usr/lib/: not a file
    /usr/local/lib/: not a file
    /usr/lib/: not a file

Because it's looking to a sibling folder, but not finding it.

Instead

prefix.install Dir["#{stage2}/*"]

And nothing else makes it work as intended. I'm not much into how native dylibs load, so perhaps someone could enlighten me as to how it would work with libexec etc, instead of sibling folders?

I'm going to fix the git checkout like @MikeMcQuaid wanted, but this PR is starting to get ready besides this particular change request.

@haf

haf commented Aug 11, 2016

Copy link
Copy Markdown
Contributor Author

In summary: how do I make the dylibs/loading work when the dotnet binary (perhaps) need to be in the root?

@dunn

dunn commented Aug 11, 2016

Copy link
Copy Markdown
Contributor

I'll look at this more closely when I can, but if installing the libraries directly into prefix is required for it to work, that's OK.

@MikeMcQuaid

Copy link
Copy Markdown
Member

I'll look at this more closely when I can, but if installing the libraries directly into prefix is required for it to work, that's OK.

Agreed as long as audit is passing.

In summary: how do I make the dylibs/loading work when the dotnet binary (perhaps) need to be in the root?

You probably need to ask upstream that question as it's not really a Homebrew one.

@haf

haf commented Aug 14, 2016

Copy link
Copy Markdown
Contributor Author

@MikeMcQuaid I'm really just asking for a link to where I can understand how the loading works on OS X with binary vs dependent library; not particularly asking for netcore.

@tdsmith tdsmith added the needs response Needs a response from the issue/PR author label Sep 3, 2016
@haf

haf commented Sep 3, 2016

Copy link
Copy Markdown
Contributor Author

Please tell me what you want changed by pointing me in the direction of the documentation that documents the APIs you want to use. You say git reference; where are those docs?

@ghost ghost removed the needs response Needs a response from the issue/PR author label Sep 3, 2016
@MikeMcQuaid

Copy link
Copy Markdown
Member

Passing on this until we have a stable version that builds without patching and passes brew audit.

@MikeMcQuaid MikeMcQuaid closed this Sep 3, 2016
@haf

haf commented Sep 3, 2016

Copy link
Copy Markdown
Contributor Author

Wow. What a dude you are Mike.

@MikeMcQuaid

Copy link
Copy Markdown
Member

@haf We're doing our best to run this project effectively.

@haf

haf commented Sep 3, 2016

Copy link
Copy Markdown
Contributor Author

@MikeMcQuaid I ask for help, you close the issue. And the lint has been passing many times over and its version is RTM, so you should not close it. You should focus on running the project productively instead, and that means taking into account lead times on change requests you make of me. If you think I've dropped this PR, then you're mistaken.

@MikeMcQuaid

Copy link
Copy Markdown
Member

A list of what needs addressed before we can include this:

  • The version may be RTM but it's mislabeled. Upstream should fix this before we include it.
  • "I'm really just asking for a link to where I can understand how the loading works on OS X with binary vs dependent library; not particularly asking for netcore." this isn't something we can really help you with as I don't understand the question and doubt if I did I'd be able to provide you with the information you need to fix this.
  • We usually don't accept new formulae that need patching to build
  • This is how you use a git checkout: http://www.rubydoc.info/github/Homebrew/brew/master/Formula.url
  • Our CI was still failing
  • This should pass all the things you checked in the original checkbox

@tdsmith

tdsmith commented Sep 3, 2016

Copy link
Copy Markdown
Contributor

wrt dynamic linkage discovery, you're probably looking the bottom of man dyld, the top of man ld, and man install_name_tool. The punchline is that OS X's linker remembers the install_name of the dylibs to which it is linked and expects those to be absolute paths, possibly including special prefixes like @rpath.

@haf

haf commented Sep 3, 2016

Copy link
Copy Markdown
Contributor Author
  1. Upstream has segregated the runtime called CoreCLR from the cli tools, and the cli tools compose your app in-place with the runtime. However, it does sound strange, I agree; that Microsoft has labelled preview2 of the cli tools as the RTM release of .Net Core, but there it is.
  2. @tdsmith thanks, that was the info I was looking for before and the link to the example means I can update this PR to match it (if you'll please reopen it)
  3. Well, we discussed that and agreed we could use git checkouts higher up in the thread, as the only reason for patching was that it builds the git checkout number into the version number.
  4. CI has been green, but since you prefer rebased PRs it's been turned red again. However, now it's gone so I can't see what it failed with any longer.
  5. And it did, and this PR is a living thing that changes as you come with feedback.

I'm looking forward to continuing this PR when it's open again.

@MikeMcQuaid MikeMcQuaid reopened this Sep 4, 2016
@BrewTestBot BrewTestBot added the in progress Stale bot should stay away label Sep 4, 2016
@ilovezfs

ilovezfs commented Sep 7, 2016

Copy link
Copy Markdown
Contributor

@haf did you find a workaround for the openssl-formula-must-be-linked problem?

@haf

haf commented Sep 13, 2016

Copy link
Copy Markdown
Contributor Author

@ilovezfs Yes, that's now part of the PR and works really nicely!

@enricosada

Copy link
Copy Markdown

/cc @blackdwarf for feedback/help about this pr

@MikeMcQuaid

Copy link
Copy Markdown
Member

@BrewTestBot test this please

@MikeMcQuaid

Copy link
Copy Markdown
Member

I reopened this 10 days ago and there's been no movement since then and CI is failing in a way that's trivially reproducible by running brew install dotnet or brew audit dotnet --new-formula on any of the 3 most recent OS X versions. Please resubmit this PR when both of those are passing, you are using a tag/release that doesn't have preview in the name and it does not require any patching to function and, ideally, no install_name_tool calls. This will very likely require working with upstream and may require another stable release from them.

To preempt some responses:

  • you are not being held to a higher standard than any other pull-request. If anything we've provided considerably more help than we normally would
  • it's disappointing to be personally criticised for closing this pull-request, reopen it and see no progress on e.g. using a git checkout which was a problem before this PR was closing and remains unaddressed.
  • these PR and formula quality standards come from running this package manager for a fairly long time. They are not personal and not a criticism of anyone but they set a bar that we need to keep maintenance time at sustainable levels and quality at an acceptable level for our users.

@BrewTestBot BrewTestBot removed the in progress Stale bot should stay away label Sep 14, 2016
This was referenced Dec 11, 2017
@Homebrew Homebrew locked and limited conversation to collaborators May 4, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants