Ensure that platform is considered for LongPlatformName instead of PackagePlatform #11900

karajas · 2017-05-24T23:25:46Z

… overriden

Microsoft.NETCore.Runtime.CoreCLR now has mscordaccore_amd64_amd64_4.6.25324.00.dll instead of mscordaccore_AnyCPU_AnyCPU_4.6.25324.00.dll.

@ellismg

ellismg · 2017-05-24T23:53:46Z

Thank you for doing this, Karthik!

/cc @ericstj @mikem8361 @gkhanna79

gkhanna79 · 2017-05-24T23:57:27Z

Thank you for fixing this - we should get this in for 2.0.0 as well.

@mikem8361 Does the resulting name for the DAC look sane to you?

ericstj · 2017-05-25T16:19:35Z

If you make this change you should do a full build with and without and diff the output. PackagePlatform/Platform have a complicated mix of interactions. In particular, make sure you didn't cause package Ids or folders to use amd64 instead of x64.

To fix a similar issue I just switched to using Platform instead. 7c53fc2

That might be the right thing to do here. /cc @chcosta

chcosta · 2017-05-25T16:41:03Z

+1 to what Eric said. The change is small, but testing this and validating all package names are unchanged / binary output locations are identical will be the majority of work. It's really easy to break this and not discover it until a week or two later. I'm not opposed to this change, I'm just stating that spending an extra day testing will save lots of headaches down the road.

karajas · 2017-05-26T00:11:57Z

Ok from diffing it looks like the only changes happen in the transport.Microsoft.NETCore.Runtime.CoreCLR packages, which is expected.

In the unchanged version I see in the nuspec

    <file src="D:\git\coreclr\bin\Product\Windows_NT.x64.Debug\mscordaccore.dll" target="runtimes\win7-x64\native\mscordaccore_AnyCPU_AnyCPU_4.6.25324.00.dll" exclude="" />
    <file src="D:\git\coreclr\bin\Product\Windows_NT.x64.Debug\sos.dll" target="runtimes\win7-x64\native\sos_AnyCPU_AnyCPU_4.6.25324.00.dll" exclude="" />

And in the changed version I see

    <file src="D:\git\coreclr\bin\Product\Windows_NT.x64.Debug\mscordaccore.dll" target="runtimes\win7-x64\native\mscordaccore_amd64_amd64_4.6.25324.00.dll" exclude="" />
    <file src="D:\git\coreclr\bin\Product\Windows_NT.x64.Debug\sos.dll" target="runtimes\win7-x64\native\sos_amd64_amd64_4.6.25324.00.dll" exclude="" />

I've only looked at the Windows_NT output
Does this seem acceptable or is there something else I should look for?

karajas · 2017-05-26T00:20:37Z

Updated this to be localized to just the area

karajas · 2017-05-26T00:21:44Z

PTAL @chcosta @ellismg @ericstj

ellismg · 2017-05-26T02:27:42Z

...Microsoft.NETCore.Runtime.CoreCLR/runtime.Windows_NT.Microsoft.NETCore.Runtime.CoreCLR.props

@@ -3,7 +3,7 @@
  <PropertyGroup>
    <PackageTargetRuntime>$(MinOSForArch)-$(PackagePlatform)</PackageTargetRuntime>
    <LongNamePlatform>$(PackagePlatform)</LongNamePlatform>
-    <LongNamePlatform Condition="'$(LongNamePlatform)'=='x64'">amd64</LongNamePlatform>
+    <LongNamePlatform Condition="'$(Platform)'=='x64'">amd64</LongNamePlatform>


Just so I understand, when we build for x86 (Windows), $(Platform) is x86 right?

Seems like you should change both of these lines rather than just the amd64 one. Test on other archs to be sure.

chcosta · 2017-05-26T16:55:59Z

LGTM (except for the CI failure)

gkhanna79 · 2017-05-30T15:51:32Z

@mikem8361 @noahfalk PTAL.

gkhanna79 · 2017-05-30T15:51:57Z

@karajas Does this change need to go into Preview2?

mikem8361 · 2017-05-30T16:39:39Z

LGTM. And yes this should go into Preview 2. Our windows debuggers look for the long names on the symbol servers. This is also a regression from 1.1.

karajas · 2017-05-30T18:40:25Z

@dotnet-bot test OSX10.12 x64 Checked Build and Test please

karajas · 2017-05-30T21:29:11Z

@ericstj Just to confirm this does not need to be applied to arm and arm64?

gkhanna79 · 2017-05-30T21:50:31Z

I think it needs to be applied to those as well. Is there a reason you think it should not be?

karajas · 2017-05-30T22:07:30Z

@gkhanna79 I was wondering if it was the earlier issue of having arm be labelled x86_arm and arm64 as amd64_arm

gkhanna79 · 2017-05-30T23:14:30Z

For Windows arm, it should be x86_arm. For Windows arm64, it should be amd64_arm64.

For Linux and OSX, we have no such cross targeting components yet.

karajas · 2017-05-31T00:11:19Z

Thanks I will update it be as such.

karajas · 2017-05-31T16:22:45Z

PTAL.

ericstj · 2017-05-31T16:37:48Z

I think you need to be looking at the diff in package content not the diff in these targets files. That will also help you understand if you're changing things you're not intending. Please share the diff of nuspecs on all platforms.

ericstj · 2017-06-01T15:30:24Z

Note that there are two renamed DACs and two SOSs for both ARM and ARM64. The ones that are native for that arch, and the ones in the cross-targeting folder.

mikem8361 · 2017-06-01T17:11:00Z

Then everything is fine and we can go ahead and commit this PR.

…ckagePlatform

karajas · 2017-06-01T17:19:11Z

@mikem8361 @ericstj Can you please check that this is the intended diff?
ARM64 - https://gist.github.com/karajas/4ddbf6387a44e21ce74aea8e197e0fe6/revisions#diff-79b3191cbdbb8781ca17f0bfef5f3689R48

ericstj · 2017-06-01T17:20:09Z

I don't think so. @mikem8361 please look at the explicit diffs for all the files. I'm pretty certain stuff is busted there.

mikem8361 · 2017-06-01T17:51:27Z

I’m not sure what is busted now. There are two sets of dlls: sos_arm64_arm64_4.6.25331.00.dll mscordaccore_arm64_arm64_4.6.25331.00.dll mscordaccore_amd64_arm64_4.6.25331.00.dll and sos_amd64_arm64_4.6.25331.00.dll The first ones should run on a arm64 device and the second one should be cross plat and run on amd64.

mikem8361 · 2017-06-01T17:55:35Z

@leculver can you check my understanding of the long naming (the previous post)?

leculver · 2017-06-01T18:04:56Z

Sorry for the delay. @mikem8361 Your explanation of the naming is correct.

We expect the arm64_arm64 to be built for the arm64 platform and run natively on those machines. We expect the AMD64_arm64 to be compiled for x64 machines (not arm machines) to debug arm64 issues.

@ericstj Can you be more specific as to what looks busted to you?

ericstj · 2017-06-01T18:19:10Z

I was referring to the previous diffs that @karajas shared in #11900 (comment), which @mikem8361 was addressing when he said #11900 (comment). I agree that the most recent diff @karajas shared for ARM64 here looks better. These previous diffs: https://gist.github.com/karajas/a75968be6fe0fd271c0e7b0bc9c0dd69/revisions#diff-544ad00592cc55861e12be35f3b7552cR49 and https://gist.github.com/karajas/a75968be6fe0fd271c0e7b0bc9c0dd69/revisions#diff-544ad00592cc55861e12be35f3b7552cR61 for ARM are what appear busted as they don't match the pattern you've described. I suspect those are just out of date since @karajas now has a new commit since I made the comment.

karajas · 2017-06-01T18:32:45Z

I will put up the full diff with the new changes for x86, x64, ARM and ARM64 shortly

karajas · 2017-06-01T21:08:15Z

Not sure why https://ci.dot.net/job/dotnet_coreclr/job/master/job/debug_windows_nt_prtest/7496/ failed

karajas · 2017-06-01T21:08:29Z

@dotnet-bot test Windows_NT x64 Debug Build and Test please

karajas · 2017-06-01T21:14:20Z

Here are the diffs with the latest changes.

x86 - https://gist.github.com/karajas/db5d316838089a8ed9aaf8b3e127f300/revisions
x64 - https://gist.github.com/karajas/3fac7cd3d670e5418edfb5689ef497cb/revisions
ARM - https://gist.github.com/karajas/924fc40ab408c9d0b9495fc656a7df32/revisions
ARM64 - https://gist.github.com/karajas/34d437457a919e83f0f4a34013250432/revisions

PTAL. @ericstj @gkhanna79 @mikem8361

gkhanna79 · 2017-06-01T21:40:05Z

Why do we have ANYCPU at https://gist.github.com/karajas/db5d316838089a8ed9aaf8b3e127f300/revisions#diff-e4c58bc4c3b8f2bb3c0dde2133eea29cR48?

gkhanna79 · 2017-06-01T21:40:19Z

And at https://gist.github.com/karajas/db5d316838089a8ed9aaf8b3e127f300/revisions#diff-e4c58bc4c3b8f2bb3c0dde2133eea29cR49?

gkhanna79 · 2017-06-01T21:44:25Z

ANYCPU on x64 diff as well.

gkhanna79 · 2017-06-01T21:45:15Z

Seems like ANYCPU is everywhere.

karajas · 2017-06-01T21:46:25Z

The red on the left is before the change and the green is what changed after the change.

gkhanna79 · 2017-06-01T21:47:36Z

You have ANYCPU in green at the very end of the change. PTAL.

karajas · 2017-06-01T21:51:13Z

Ah I see what's happening the line you clicked on was in the initial revision when I added the file. When you click on a line in the older revision, it looks like it picks up the same line in the current revision so that's why I was confused.

That green AnyCPU on the right is from creating the initial gist with the package output prior to the change.

karajas · 2017-06-01T21:56:48Z

The important diffs are all on the runtime.win*.microsoft.runtime.coreclr.nuspec file, which is here

x86 - https://gist.github.com/karajas/db5d316838089a8ed9aaf8b3e127f300/revisions#diff-e4c58bc4c3b8f2bb3c0dde2133eea29c
x64 -https://gist.github.com/karajas/3fac7cd3d670e5418edfb5689ef497cb/revisions#diff-22c998476af00604e7254ae5c5d6352d
ARM - https://gist.github.com/karajas/924fc40ab408c9d0b9495fc656a7df32/revisions#diff-544ad00592cc55861e12be35f3b7552c
ARM64 -https://gist.github.com/karajas/34d437457a919e83f0f4a34013250432/revisions#diff-79b3191cbdbb8781ca17f0bfef5f3689

karajas · 2017-06-01T22:43:41Z

Can I get sign off on this change?

gkhanna79 · 2017-06-01T22:44:20Z

@mikem8361 Can you signoff on this if this looks good to you?

karajas · 2017-06-01T22:53:25Z

Got an offline LGTM.

mikem8361 · 2017-06-01T22:56:24Z

LGTM

dnfclas added the cla-already-signed label May 24, 2017

karajas force-pushed the fixDacDll branch from d6eabff to 7fdfed7 Compare May 24, 2017 23:31

ellismg requested review from chcosta and wtgodbe May 24, 2017 23:54

karajas force-pushed the fixDacDll branch from 7fdfed7 to 9f1a79f Compare May 26, 2017 00:18

karajas changed the title ~~Allow PackagePlatform to be used by LongPlatformName instead of being…~~ Ensure that platform is considered for LongPlatformName instead of PackagePlatform May 26, 2017

ellismg reviewed May 26, 2017

View reviewed changes

karajas force-pushed the fixDacDll branch from 9f1a79f to 3485333 Compare May 30, 2017 21:32

karajas force-pushed the fixDacDll branch 3 times, most recently from ea747a8 to 44c6219 Compare May 31, 2017 16:22

Ensure that platform is considered for LongPlatformName instead of Pa…

62db223

…ckagePlatform

karajas force-pushed the fixDacDll branch from fbbac57 to 62db223 Compare June 1, 2017 17:52

karajas mentioned this pull request Jun 1, 2017

Ensure that platform is considered for LongPlatformName instead of PackagePlatform #12042

Merged

karajas merged commit 91329ce into dotnet:master Jun 1, 2017

karelz modified the milestone: 2.1.0 Aug 28, 2017

Ensure that platform is considered for LongPlatformName instead of PackagePlatform #11900

Ensure that platform is considered for LongPlatformName instead of PackagePlatform #11900

Conversation

karajas commented May 24, 2017 • edited Loading

ellismg commented May 24, 2017

gkhanna79 commented May 24, 2017

ericstj commented May 25, 2017 • edited Loading

chcosta commented May 25, 2017

karajas commented May 26, 2017 • edited Loading

karajas commented May 26, 2017

karajas commented May 26, 2017

ellismg May 26, 2017

Choose a reason for hiding this comment

ericstj May 26, 2017 • edited Loading

Choose a reason for hiding this comment

chcosta commented May 26, 2017

gkhanna79 commented May 30, 2017

gkhanna79 commented May 30, 2017

mikem8361 commented May 30, 2017

karajas commented May 30, 2017

karajas commented May 30, 2017

gkhanna79 commented May 30, 2017

karajas commented May 30, 2017

gkhanna79 commented May 30, 2017

karajas commented May 31, 2017

karajas commented May 31, 2017

ericstj commented May 31, 2017

ericstj commented Jun 1, 2017

mikem8361 commented Jun 1, 2017

karajas commented Jun 1, 2017

ericstj commented Jun 1, 2017

mikem8361 commented Jun 1, 2017 via email

mikem8361 commented Jun 1, 2017

leculver commented Jun 1, 2017

ericstj commented Jun 1, 2017

karajas commented Jun 1, 2017

karajas commented Jun 1, 2017

karajas commented Jun 1, 2017

karajas commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

karajas commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

karajas commented Jun 1, 2017 • edited Loading

karajas commented Jun 1, 2017

karajas commented Jun 1, 2017

gkhanna79 commented Jun 1, 2017

karajas commented Jun 1, 2017

mikem8361 commented Jun 1, 2017

karajas commented May 24, 2017 •

edited

Loading

ericstj commented May 25, 2017 •

edited

Loading

karajas commented May 26, 2017 •

edited

Loading

ericstj May 26, 2017 •

edited

Loading

karajas commented Jun 1, 2017 •

edited

Loading