Skip to content

Cross-compile RemoteExecutor#2402

Merged
ViktorHofer merged 7 commits intodotnet:masterfrom
ViktorHofer:RemoteExecutorTfms
May 5, 2019
Merged

Cross-compile RemoteExecutor#2402
ViktorHofer merged 7 commits intodotnet:masterfrom
ViktorHofer:RemoteExecutorTfms

Conversation

@ViktorHofer
Copy link
Copy Markdown
Member

Instead of using a single netstandard2.0 tfm and putting tfm specific stuff together (runtimeconfig.json which currently also gets deployed when referencing the package from netfx) cross-compiling both libraries. I still had to manually add a runtimeconfig.json as otherwise it wouldn't get deployed in the ProjectReference with PrivateAssets=All. Also I wasn't able to get the deps.json files also copied over.

@dasMulli any idea with PrivateAssets="All" on a ProjectReference doesn't include the $(TargetName).runtimeconfig.json and the $(TargetName).deps.json assets?

@ViktorHofer ViktorHofer self-assigned this Apr 2, 2019
@ViktorHofer ViktorHofer requested a review from ericstj April 2, 2019 19:20
@dasMulli
Copy link
Copy Markdown

dasMulli commented Apr 4, 2019

The build and publish runtime configurations don't flow across P2Ps, they are part of the "PrepareForRun" step and not included in the targets that assemble the copy local items for P2Ps references. The .deps.json file is also generated separately during publish since it may need to contain different info for build/publish.

When packing assemblies via P2Ps i previously resorted to packing publish outputs (see dotnet/sdk#1846 (comment)) or messing with project references so that they can forward their publish output.. see https://github.com/dasMulli/AssemblyInfoGenerationSdk/blob/master/src/DasMulli.AssemblyInfoGeneration.Sdk/sdk-layout.targets (the test project references the SDK project as P2P with a special property - which will do a publish and forward the publish result, with the added complexity of referencing the outer bulid of a multi-targeting project and forwarding a custom layout)

@ViktorHofer
Copy link
Copy Markdown
Member Author

@dasMulli thanks a lot for letting me know! Packaging the publish out is what we do in some other packages here in arcade. But we usually only do this for build tools that land in the tools directory. I think I will stick with the current solution.

@ericstj PTAL

@ViktorHofer ViktorHofer requested a review from ericstj April 12, 2019 19:26
@ViktorHofer
Copy link
Copy Markdown
Member Author

@ericstj this is ready to go in. awaiting final review :)

<ItemGroup>
<TfmSpecificPackageFile Include="$(OutputPath)Microsoft.DotNet.RemoteExecutorHost.dll" PackagePath="lib\$(TargetFramework)\" />
<TfmSpecificPackageFile Include="$(OutputPath)Microsoft.DotNet.RemoteExecutorHost.runtimeconfig.json" PackagePath="lib\$(TargetFramework)\" />
<TfmSpecificPackageFile Include="%(_ResolvedProjectReferencePaths.Identity)" PackagePath="tools\$(TargetFramework)\" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of using this private item, you could set OutputItemType on the projectreference. https://blogs.msdn.microsoft.com/kirillosenkov/2015/04/04/how-to-have-a-project-reference-without-referencing-the-actual-binary/

Copy link
Copy Markdown
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

LGTM modulo one minor issue.

@ViktorHofer
Copy link
Copy Markdown
Member Author

OK addressed your feedback. Package content:

image

@ViktorHofer
Copy link
Copy Markdown
Member Author

Seems like I'm missing something for the .exe.config copying. Probably a dependent target I need for the manual packaging.

@ViktorHofer ViktorHofer force-pushed the RemoteExecutorTfms branch from 8c01ac7 to 20c7dea Compare May 5, 2019 13:08
@ViktorHofer ViktorHofer merged commit 6a34948 into dotnet:master May 5, 2019
@ViktorHofer ViktorHofer deleted the RemoteExecutorTfms branch May 5, 2019 13:29
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.

3 participants