Use all RIDs on all platforms #23113

khyperia · 2017-11-10T16:31:15Z

Ping @jaredpar - I have absolutely no idea why this was the way it is, so I figured I'd try removing it and see what happens.

Also, I don't know what UseSharedCompilation controls, but I figured it's the compiler server, and hey that works on linux now.

Also, no idea what BaseNuGetRuntimeIdentifier is, but things seem to work without it (seems to be something related to the old project system).

Pilchie · 2017-11-10T16:35:42Z

build/Targets/Settings.props

@@ -34,8 +34,9 @@
    <RoslynDiagnosticsPropsFilePath>$(NuGetPackageRoot)/microsoft.net.roslyndiagnostics/$(RoslynDiagnosticsNugetPackageVersion)/build/Microsoft.Net.RoslynDiagnostics.props</RoslynDiagnosticsPropsFilePath>
    <RoslynPortableTargetFrameworks>net461;netcoreapp2.0</RoslynPortableTargetFrameworks>
    <RoslynPortableTargetFrameworks46>net46;netcoreapp2.0</RoslynPortableTargetFrameworks46>
-    <RoslynPortableRuntimeIdentifiers>win7;win7-x64</RoslynPortableRuntimeIdentifiers>
-
+    <RoslynPortableRuntimeIdentifiers>win7;win7-x64;linux-x64;osx-x64</RoslynPortableRuntimeIdentifiers>


👍 This is actually more correct, since the expectation is normally that you shouldn't have to restore per-config.

Pilchie · 2017-11-10T16:36:25Z

build/Targets/Settings.props

@@ -141,8 +146,6 @@

  <!-- Windows specific settings -->
  <PropertyGroup Condition="'$(OS)' == 'Windows_NT'">
-    <BaseNuGetRuntimeIdentifier>win7</BaseNuGetRuntimeIdentifier>


Pretty sure this was a workaround for a bug that got fixed a long time ago, where things didn't work properly for anycpu configs.

Yes. Kill this with fire.

You might be able to kill more of this with fire as well...

jaredpar

Pending @tmat feedback on whether we should just make the PDB story completely consistent across all OS now.

jaredpar · 2017-11-10T16:47:28Z

build/Targets/Settings.props

@@ -141,8 +146,6 @@

  <!-- Windows specific settings -->
  <PropertyGroup Condition="'$(OS)' == 'Windows_NT'">
-    <BaseNuGetRuntimeIdentifier>win7</BaseNuGetRuntimeIdentifier>


Yes. Kill this with fire.

jaredpar · 2017-11-10T16:49:43Z

build/Targets/Settings.props

    -->
    <RoslynDebugType Condition="'$(OfficialBuild)' == 'true'">portable</RoslynDebugType>
    <RoslynDebugType Condition="'$(OfficialBuild)' != 'true'">embedded</RoslynDebugType>
+    <RoslynDebugType Condition="'$(OS)' != 'Windows_NT'">portable</RoslynDebugType>


Should we revisit this logic now? I think it's possible for the Unix builds to now be completely consistent with our Windows builds here for PDBs. The logic for embedding and portable should work in both places.

CC @tmat as he's been working on this area.

I agree. There is no need for different settings on Windows.

In reply to: 150285679 [](ancestors = 150285679)

jasonmalinowski · 2017-11-10T17:11:40Z

build/Targets/Settings.props

@@ -34,8 +34,9 @@
    <RoslynDiagnosticsPropsFilePath>$(NuGetPackageRoot)/microsoft.net.roslyndiagnostics/$(RoslynDiagnosticsNugetPackageVersion)/build/Microsoft.Net.RoslynDiagnostics.props</RoslynDiagnosticsPropsFilePath>
    <RoslynPortableTargetFrameworks>net461;netcoreapp2.0</RoslynPortableTargetFrameworks>
    <RoslynPortableTargetFrameworks46>net46;netcoreapp2.0</RoslynPortableTargetFrameworks46>
-    <RoslynPortableRuntimeIdentifiers>win7;win7-x64</RoslynPortableRuntimeIdentifiers>
-
+    <RoslynPortableRuntimeIdentifiers>win7;win7-x64;linux-x64;osx-x64</RoslynPortableRuntimeIdentifiers>


Should that be win7-x86;win7-x64;...? It'll probably work as long as we don't have any native dependencies but would be slightly more correct I think?

(yes, I recognize it's the wrong thing already, but thought I'd ask...)

I think this is another hold over from the project.json days. I wonder if we could just make it win7 flat? But I've never been super strong an RIDs.

We could explore that in another PR though. This is def moving us forward as is.

AFAIK win7 means win7-x86. /cc @weshaggard

The win7 RID (and any RID that doesn't have an architecture) means it works for all architectures (or is architecture-agnostic). Check out https://github.com/dotnet/corefx/tree/master/pkg/Microsoft.NETCore.Platforms#runtime-ids for a really good explanation of RIDs.

BTW - win7-x64 can be changed to just win-x64, since that is the "portable" RID for Windows.

jasonmalinowski · 2017-11-10T17:12:29Z

build/Targets/Settings.props

@@ -141,8 +146,6 @@

  <!-- Windows specific settings -->
  <PropertyGroup Condition="'$(OS)' == 'Windows_NT'">
-    <BaseNuGetRuntimeIdentifier>win7</BaseNuGetRuntimeIdentifier>


You might be able to kill more of this with fire as well...

It turns out the BaseNuGetRuntimeIdentifier is still needed on two projects which are not fully converted to the new SDK. Moving the property to those specific projects. It will go away once they are converted to the new SDK.

khyperia · 2017-11-13T16:41:16Z

BTW - win7-x64 can be changed to just win-x64, since that is the "portable" RID for Windows.

I just pushed a commit that did this - if it Just Works, I'll keep it, but I'm not too confident in it. There are a lot of uses of win7.

khyperia · 2017-11-13T16:43:19Z

#22984 (comment) @jaredpar

Wait ... we still have a project.json file???? I will delete this soon.


% find . -name project.json
./src/Deployment/DeployToolsetCompiler/project.json
./src/Setup/BuildTasks/project.json
./src/Tools/Github/GitMergeBot/project.json
./src/Tools/Source/DebuggerVisualizers/project.json
./src/Tools/Source/MetadataVisualizer/project.json

💖

jaredpar · 2017-11-13T16:42:27Z

build/Targets/Settings.props

-    <RoslynPortableRuntimeIdentifiers>win7;win7-x64</RoslynPortableRuntimeIdentifiers>
-
+    <RoslynPortableRuntimeIdentifiers>win;win-x64;linux-x64;osx-x64</RoslynPortableRuntimeIdentifiers>
+    <RoslynDesktopRuntimeIdentifiers>win</RoslynDesktopRuntimeIdentifiers>


jaredpar · 2017-11-13T16:44:57Z

build/Targets/Roslyn.Toolsets.Xunit.targets

@@ -16,7 +16,7 @@
    <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects>
    <ForceGenerationOfBindingRedirects>true</ForceGenerationOfBindingRedirects>

-    <RuntimeIdentifier Condition="'$(TargetFramework)' == 'net461' AND '$(RuntimeIdentifier)' == '' AND '$(OS)' == 'Windows_NT'">win7</RuntimeIdentifier>
+    <RuntimeIdentifier Condition="'$(TargetFramework)' == 'net461' AND '$(RuntimeIdentifier)' == '' AND '$(OS)' == 'Windows_NT'">$(RoslynDesktopRuntimeIdentifiers)</RuntimeIdentifier>


Don't think we can use this property here. The $(RoslynDesktopRuntimeIdentifiers) is used in the <RuntimeIdentifiers> (emphasis on the plural form) position in the build files. This particular property is <RuntimeIdentifier> (the single form). If the <RoslynDesktopRuntimeIdentifiers> property ever switched to multi-targeting this would be invalid.

Suggest that for now we change the $(RoslynRuntimeIdentifier) to a singular form.

Is there a reason why we can't multitarget this instead? (change the prop to RuntimeIdentifiers).

VisualStudioInteractiveComponents.csproj is also single-targeted.

khyperia · 2017-11-13T21:25:48Z

retest windows_debug_unit64_prtest please

Use all RIDs on all platforms

d902abb

Pilchie requested a review from jasonmalinowski November 10, 2017 16:34

Pilchie approved these changes Nov 10, 2017

View reviewed changes

jaredpar approved these changes Nov 10, 2017

View reviewed changes

jasonmalinowski approved these changes Nov 10, 2017

View reviewed changes

jaredpar and others added 3 commits November 10, 2017 09:51

Add BaseNuGetRuntimeIdentifier to a few projects

1da2473

It turns out the BaseNuGetRuntimeIdentifier is still needed on two projects which are not fully converted to the new SDK. Moving the property to those specific projects. It will go away once they are converted to the new SDK.

Remove special casing for Linux/Mac debug type

a34df92

Replace win7 RID with win

aa651be

jaredpar reviewed Nov 13, 2017

View reviewed changes

Undo potential multitargeting of some projects

464af55

khyperia merged commit b63240f into dotnet:post-dev15.5-contrib Nov 13, 2017

khyperia mentioned this pull request Nov 14, 2017

Replace Windows RID multitargeting with singular form #23161

Merged

khyperia deleted the rid_all branch November 14, 2017 17:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use all RIDs on all platforms #23113

Use all RIDs on all platforms #23113

khyperia commented Nov 10, 2017

Pilchie Nov 10, 2017

Pilchie Nov 10, 2017

jaredpar Nov 10, 2017

jasonmalinowski Nov 10, 2017

jaredpar left a comment

jaredpar Nov 10, 2017

jaredpar Nov 10, 2017

tmat Nov 10, 2017

jasonmalinowski Nov 10, 2017

jasonmalinowski Nov 10, 2017

jaredpar Nov 10, 2017

terrajobst Nov 10, 2017

eerhardt Nov 10, 2017

jasonmalinowski Nov 10, 2017

khyperia commented Nov 13, 2017

khyperia commented Nov 13, 2017

jaredpar Nov 13, 2017

jaredpar Nov 13, 2017

khyperia Nov 13, 2017

khyperia commented Nov 13, 2017

Use all RIDs on all platforms #23113

Use all RIDs on all platforms #23113

Conversation

khyperia commented Nov 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaredpar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia commented Nov 13, 2017

khyperia commented Nov 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khyperia commented Nov 13, 2017