Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Overhaul external / binplacing / reference from runtime #15705

Merged
merged 4 commits into from
Feb 2, 2017

Conversation

ericstj
Copy link
Member

@ericstj ericstj commented Feb 2, 2017

Best to review commit-by-commit.

First change I made was to switch external to use binplacing logic. Let let us have better control over when to copy and where and allowed copying to multiple locations easily. As part of this I binplace both the primary output (if it exist) as well as all copy-local items.

Second change was to combine coreclr and NETNative into a common runtime project (thanks @joperezr). This was to enable the following change.

Third I fixed ReferenceFromRuntime to use a project reference to the runtime project for src projects, and block its use from tests. This enables us to build multiple configurations at a time that may need different runtime libs without stomping on each other in the runtime path. It also fixes cases where folks might accidentally use ReferenceFromRuntime instead of a project reference.

Finally I noticed that the first change caused a bunch of clashing copies to occur. To fix this (which should be general goodness) I disabled copy local for project references for anything that isn't a test project.

/cc @weshaggard @tarekgh

@@ -2,8 +2,6 @@
<Project ToolsVersion="14.0" xmlns="http://schemas.microsoft.com/developer/msbuild/2003" DefaultTargets="Build">
<Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
<PropertyGroup>
<OutputPath>$(RuntimePath)</OutputPath>
<!-- WORKAROUND: Force external packages to be restored for x64 until arm packages are fully broughtup-->
Copy link
Contributor

Choose a reason for hiding this comment

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

Intentionally removed the comment?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the file was consolidated, but maybe we still want the comment in the new location (runtime.depproj)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can add that back, removing the comment was accidental.

Copy link
Member Author

Choose a reason for hiding this comment

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

I doctored up the commits to bring that comment back 👍

@mellinoe
Copy link
Contributor

mellinoe commented Feb 2, 2017

This LGTM. The previous functionality was indeed dangerous if you used it to reference anything violating the project build ordering, and there was nothing stopping that from happening.

This switches the external dir to binplace rather than copy to output.

This is more flexible as it will conditionally binplace to many
locations based on what's needed for the configuration.
Combine these so we have a common runtime project.
Thanks to @joperezr for this change.
ReferenceFromRuntime for src projects isn't safe.  It allows any source
project to reference the output of another source project without
properly sequencing the build.

This fixes that by changing ReferenceFromRuntime to instead use a
ProjectReference to the runtime project for the building configuration
and filter that to just the files requested.

I also completely block the use of ReferenceFromRuntime in test projects
@ericstj ericstj merged commit 327f4e5 into dotnet:master Feb 2, 2017
@karelz karelz modified the milestone: 2.0.0 Feb 4, 2017
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Overhaul external / binplacing / reference from runtime

Commit migrated from dotnet/corefx@327f4e5
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants