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

Enable RuntimeInformation intrinsic functions on all platforms #1926

Merged
merged 5 commits into from
Mar 31, 2017

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Mar 29, 2017

No description provided.

@eerhardt
Copy link
Member

This basically fixes #539 right? You can detect which OS you are on with this.

@@ -1,4 +1,7 @@
{
"dependencies": {
"System.Runtime.InteropServices.RuntimeInformation": "4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Be sure to add the assembly to our custom copy logic here: https://github.com/cdmihai/msbuild/blob/fa2c0f8541ed96a303927131abe268952bf37706/src/Build/Microsoft.Build.csproj#L798

You might need to also add it to the build definition so that it ends up in our VSIX.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch ... I'll also update the swr and microbuild if needed

@cdmihai
Copy link
Contributor Author

cdmihai commented Mar 30, 2017

This basically fixes #539 right? You can detect which OS you are on with this.

@eerhardt I think so, yes. In the future we could think about adding intrinsic functions like IsLinux / IsOSX / IsWindows to avoid screen wide lines.

@cdmihai
Copy link
Contributor Author

cdmihai commented Mar 30, 2017

@dotnet-bot test Windows_NT Build for Desktop please

@cdmihai
Copy link
Contributor Author

cdmihai commented Mar 30, 2017

@dotnet-bot test Windows_NT Build for CoreCLR please

@radical
Copy link
Member

radical commented Mar 31, 2017

@cdmihai Thanks! One item off my list ;)

The Reference items produced by ResolveNugetPackages target during project builds are either reference assemblies or real assemblies. Therefore they are not a good source for copying into the output directory, as copied reference assemblies would crash at runtime.

Move the copying in the DeployDependencies project file because the DeployRuntime target in there uses the real assemblies.

This should all be removed whenever we move to dotnet publish
@@ -19,12 +19,14 @@
<dependency id="Microsoft.Build.Framework" version="[$version$]" />
<dependency id="Microsoft.Build.Tasks.Core" version="[$version$]" />
<dependency id="Microsoft.Build.Utilities.Core" version="[$version$]" />
<dependency id="System.Runtime.InteropServices.RuntimeInformation" version="4.0.0" />
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't need to add this to our runtime package because the dependency on Microsoft.Build transitively depends on System.Runtime.InteropServices.RuntimeInformation.

Copy link
Contributor Author

@cdmihai cdmihai Mar 31, 2017

Choose a reason for hiding this comment

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

MSBuild.exe depends on RuntimeInformation via the MSBuildEnvironmentHelper, which is compiled into MSBuild.exe. Because of that, I thought it might be better to make the dependency explicit.
On the other hand, everything else used by msbuild.exe package flows from the other packages. so I'll remove it for consistency's sake :)

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.

5 participants