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

Update mono to 5.10.1.20 #1137

Merged
merged 5 commits into from
Apr 5, 2018
Merged

Conversation

DustinCampbell
Copy link
Contributor

@DustinCampbell DustinCampbell commented Mar 16, 2018

This PR takes a new Mono that includes a new platform-specific binary: libMonoPosixHelper.[so|dylib]. This binary is needed for Cake to properly unzip NuGet packages. Otherwise, the Cake tests hang.

@filipw
Copy link
Member

filipw commented Mar 16, 2018

perfect - thanks!

@DustinCampbell DustinCampbell changed the title Update embedded mono to 5.10.0.170 Update embedded mono to 5.10.1.20 Apr 3, 2018
@DustinCampbell DustinCampbell changed the title Update embedded mono to 5.10.1.20 Update mono to 5.10.1.20 Apr 4, 2018
@DustinCampbell
Copy link
Contributor Author

Ugh. I've made Linux hang now. 😄

@DustinCampbell
Copy link
Contributor Author

OK. It looks like the Linux hang is a good thing that revealed another problem. Essentially, Cake tries to launch its Cake.Bakery.exe on the same local Mono that the parent process was launched with. However, it doesn't include any of the arguments that were passed to the local Mono, such as --assembly-loader=strict and --config ${CONFIG_FILE}. It's the latter argument that's particularly important because it tells the Mono runtime how to handle various native binaries for DLL imports. I've now updated our run script to set the MONO_ENV_OPTIONS variable with those arguments rather than passing them in directly. That variable should be inherited by the child Cake.Bakery.exe process and everything should be happy.

It really seems like Cake.Bakery.exe has been working somewhat by accident all of this time. 😄

@DustinCampbell
Copy link
Contributor Author

cc @mholo65

Copy link
Member

@filipw filipw left a comment

Choose a reason for hiding this comment

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

wow what an obscure thing to find! 🕵️

@DustinCampbell
Copy link
Contributor Author

To give credit where credit is due, @mholo65 found the smoking gun. However, getting to the bottom of it was an ordeal.

@DustinCampbell DustinCampbell merged commit 4498c8d into OmniSharp:master Apr 5, 2018
@DustinCampbell
Copy link
Contributor Author

@rchande, @akshita31, @TheRealPiotrP: With this PR, the layout of the *nix OmniSharp releases (e.g. osx, Linux-x86 and linux-x64) have changed a bit. The run script is in the same place, but the Mono runtime itself moved into a bin folder. At first, I thought this might require some tweaking to downloader code in omnisharp-vscode. Now I'm thinking that everything should "just work".

I was concerned because we list the Mono runtime in the package.json as one of the "binaries" for each of the *nix OmniSharp runtime dependencies:

The "binaries" entry is used by the package downloader to determine what files should have executable permissions after the package is downloaded and unzipped. The logic to set these permissions is here.

Looking at that logic, it appears that the downloader will continue to successful. That is, it won't result in an error if one of the "binaries" doesn't actually exist. However, it won't give the proper permissions to the Mono runtime because its been moved to "bin". To handle this problem (and future-proof it a bit), I've also updated the "run" script (which hasn't moved) to run chmod 755 on the Mono runtime. Essentially, only the "run" script needs executable permissions. When it runs, it will set whatever permissions need to be set to function.

So, no changes are needed for omnisharp-vscode. However, we should keep the Mono runtime listed in the package.json to allow older OmniSharp releases to continue working.

Sorry if that was a bit convoluted. Did it make sense?

@bjorkstromm
Copy link
Member

Thanks @DustinCampbell!

Curious, do you happen to know if this is needed anymore? When doing the Cake PR, we had problems with hangs due to external processes (Cake.Bakery.exe) was not started using the run script in the embedded mono. Setting the MONO_ENV_OPTIONS and MONO_PATH should do the trick, right?

@DustinCampbell
Copy link
Contributor Author

I think it's still needed, isn't it? Otherwise, wouldn't it launch Cake.Bakery.exe using the Mono runtime on the PATH? If so, that would likely have disastrous results when combined with those environment variables. For example, that would cause it to run on a Mono runtime that likely wouldn't match the mscorlib located in MONO_PATH.

@bjorkstromm
Copy link
Member

IIRC, when doing Process.Start() Mono looks for the PE header of the file to determine whether it's a .NET executable, and if it's it will start it using same Mono runtime.

@DustinCampbell
Copy link
Contributor Author

I suppose that would make sense. 😄

@rchande
Copy link

rchande commented Apr 5, 2018

@DustinCampbell Thanks for working through this!
With regard to your comment:

So, no changes are needed for omnisharp-vscode. However, we should keep the Mono runtime listed in the package.json to allow older OmniSharp releases to continue working.

I suspect we could update the "binaries" values to the correct locations once we consume a newer omnisharp in C# for VS Code. Of course, that would require users of that version of the extension to only use omnisharp published after it. Thoughts?

@DustinCampbell
Copy link
Contributor Author

@rchande: I have no problem with that. I also have no problem leaving it there for all of eternity. 😄

@DustinCampbell DustinCampbell deleted the updates branch April 11, 2018 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants