enable build System.Private.Interop project by luqunl · Pull Request #4836 · dotnet/corert

luqunl · 2017-10-30T17:36:50Z

verify the fix for build issue

luqunl · 2017-10-30T19:39:53Z

MichalStrehovsky · 2017-10-30T20:51:30Z

Can you confirm these two projects no longer place their build outputs in the same directory? We had build races in the official build because of that (not the CI build; the build that does signing and produces NuGet packages).

I'm still missing the broader context on these though. Are we going to ship NuGet packages of these from this repo? Or why do these project files need to build here? The build is not really set up for this - e.g. right now these libraries are going to be placed in the CoreRT SDK even though they're unused because they don't actually target CoreRT.

I'm not opposed to making changes to enable these scenarios, but it might need some broader build changes, not just hacking up OutputPath to something else.

luqunl · 2017-10-31T18:58:09Z

@MichalStrehovsky resolve two projects' build outputs in the same directory issue-- the problem is related to import a project file instead of target file.

luqunl · 2017-10-31T18:59:16Z

Are we going to ship NuGet packages of these from this repo?
// Yes, some of our customers require to use NuGet packages

luqunl · 2017-10-31T19:06:10Z

why do these project files need to build here?
// Customer want to use open-source System.Private.Interop. To open source System.Private.Interop, there isn't many options for us.

in GitHub CoreRT repro
Pro:
it takes less time to add/build/sign/publish/test these projects into CoreRT repro.
Con:
These projects doesn't 'really' belong to CoreRT.
in GitHub 'new' repro
Pro:
These projects fits better in the 'new' repro, such as Interop Repro
Con:
It take much longer time to add/build/sign/publish/test these projects.

In future, We will consider Option #2.

MichalStrehovsky · 2017-10-31T20:10:58Z

Customer want to use open-source System.Private.Interop

@luqunl There are many ways how to achieve that. E.g. the mono repo already uses the CoreRT repo as a submodule and they pick and choose files they would like to use. What's the intended consumption model here?

luqunl · 2017-10-31T20:18:05Z

@MichalStrehovsky The consumption mode is that customers can just use nuget package generated from CoreRT branch and can submit PR for fixing S.P.Interop issues

MichalStrehovsky · 2017-10-31T20:35:37Z

OK, so we need official builds and NuGet packaging.

This will require some broader thinking on your part. If these projects are just hooked up into whatever build configurations CoreRT repo defines, there will be issues with build matrix. Mono's and CoreCLR's build matrix is different from CoreRT. The way we have the alternate S.P.Interop projects hooked up into build.cmd right now means that you can't produce a e.g. x86 build of the S.P.Interop library without making all of CoreRT compile on x86 first (see #4589 - not even x86 is supported right now in CoreRT). Especially Mono's support matrix is huge and spans architectures and operating systems.

luqunl · 2017-10-31T20:41:00Z

System.Private.Interop for mono only works on Windows. so its matrix for using System.Private.Interop should be similar as CoreRT.

luqunl · 2017-10-31T20:42:15Z

in my mind, x86/amd64/arm build configurations for System.Private.Interop should be enough

jkotas · 2017-10-31T20:45:05Z

@luqunl As we have discussed during our phone call with Mono team, they are not going to consume NuGet packages from CoreRT repo. They are going to build their own binaries from CoreRT sources.

I see the Mono build flavor here just as a sanity check that things are not completely broken.

jkotas · 2017-10-31T20:47:18Z

    <AssemblyName>System.Private.Interop</AssemblyName>
    <TargetName>$(AssemblyName)</TargetName>
-    <DefineConstants>$(DefineConstants);ENABLE_MIN_WINRT</DefineConstants>
+    <OutputType>Library</OutputType>


This is going to drop the binaries under sdk\mono. I would like everything under sdk to be CoreRT runtime/toolchain specific. Could you please drop these files under e.g. interop.

do you mean interop folder and sdk folder are under same parent folder? like
sdk
interop

luqunl · 2017-10-31T21:02:15Z

@jkotas, Azure Service Fabric and VS Debugger team want to consume from Nuget package

jkotas · 2017-10-31T21:12:39Z

Right, they are different from Mono. I would expect we will build one platform-neutral build for them.

As @MichalStrehovsky said, it will require some build plumbing to get the build and packaging right.

luqunl · 2017-10-31T21:20:35Z

@jkotas, some of interop code are architecture dependent. so neutral doesn't work for them.
such as

corert/src/System.Private.Interop/src/System/Runtime/InteropServices/Marshal.cs

Line 374 in 53f2da1

#if WIN32

jkotas · 2017-10-31T21:23:38Z

Then you will need to choose between making this platform neutral, e.g. by using if (sizeof(int) == sizeof(IntPtr)) instead of ifdef in this case, or even more build infrastructure plumbing.

only enable mini winrt on windows OS and resolve build output folder overlap problem between CoreCLR and Mono Please enter the commit message for your changes. Lines starting Change output folder to {root}\interop

luqunl · 2017-10-31T21:34:08Z

For build infrastructure plumbing, beside support build neutral assembly, Is there anything else specific for these two projects?
In short team, In my mind, it isn't critical to enable support build neural assembly.

jkotas · 2017-10-31T21:48:05Z

If you do not have platform neutral build, you will pay for it in the packaging and build leg complexity. Up to you where you want to pay the cost...

Either way, you may want to have separate build.cmd for the official build of the interop bits so that you do not have to build the whole corert repo just to build the interop bits.

luqunl · 2017-10-31T22:10:21Z

Thanks for your explanation. if these two(neutral/build.cmd) becomes problem, then we can fix them one by one.

jkotas · 2017-10-31T22:25:10Z

@luqunl We prefer to use squash&merge option to merge regular PRs.

luqunl · 2017-11-01T03:50:06Z

Next time, I will follow this.

luqunl added the * NO MERGE * label Oct 30, 2017

dnfclas added the cla-already-signed label Oct 30, 2017

luqunl force-pushed the buildissue branch from 659f612 to 9f3c906 Compare October 30, 2017 18:28

luqunl requested a review from MichalStrehovsky October 30, 2017 19:21

luqunl removed the * NO MERGE * label Oct 30, 2017

luqunl force-pushed the buildissue branch from 9f3c906 to 92b1ef4 Compare October 31, 2017 18:55

luqunl force-pushed the buildissue branch from 92b1ef4 to a9eb61e Compare October 31, 2017 20:13

jkotas suggested changes Oct 31, 2017

View reviewed changes

Enable build System.Private.Interop.mono project

3679a5e

only enable mini winrt on windows OS and resolve build output folder overlap problem between CoreCLR and Mono Please enter the commit message for your changes. Lines starting Change output folder to {root}\interop

luqunl force-pushed the buildissue branch from a9eb61e to 3679a5e Compare October 31, 2017 21:24

Use low case interop

4fc5070

jkotas approved these changes Oct 31, 2017

View reviewed changes

luqunl merged commit 40d48f0 into dotnet:master Oct 31, 2017

Conversation

luqunl commented Oct 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luqunl commented Oct 30, 2017

Uh oh!

MichalStrehovsky commented Oct 30, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

MichalStrehovsky commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

MichalStrehovsky commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

jkotas commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

luqunl Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

jkotas Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

luqunl Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

jkotas commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luqunl commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkotas commented Oct 31, 2017

Uh oh!

luqunl commented Oct 31, 2017

Uh oh!

jkotas commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luqunl commented Nov 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

luqunl commented Oct 30, 2017 •

edited

Loading

jkotas commented Oct 31, 2017 •

edited

Loading

jkotas commented Oct 31, 2017 •

edited

Loading

luqunl commented Oct 31, 2017 •

edited

Loading

luqunl commented Oct 31, 2017 •

edited

Loading

jkotas commented Oct 31, 2017 •

edited

Loading