Skip to content

Add SourceRootMappedPathsFeatureSupported property#26095

Merged
tmat merged 2 commits intodotnet:masterfrom
tmat:SourceRootMappedPaths
Apr 12, 2018
Merged

Add SourceRootMappedPathsFeatureSupported property#26095
tmat merged 2 commits intodotnet:masterfrom
tmat:SourceRootMappedPaths

Conversation

@tmat
Copy link
Copy Markdown
Member

@tmat tmat commented Apr 11, 2018

Customer scenario

Add a property to Microsoft.Managed.Core.targets that declares that target InitializeSourceRootMappedPaths is available. This property allows SourceLink package targets to detect whether or not to depend on this target and avoid the dependency if the compiler doesn't support the target (e.g. F# projects, projects using older C# compiler version). Without this property available SourceLink package would fail with with a cryptic build error that doesn't tell the customer anything useful.

Bugs this fixes

https://devdiv.visualstudio.com/DevDiv/_workitems/edit/598723

Workarounds, if any

No reasonable workaround.

Risk

Small.

Performance impact

None.

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

SourceLink package testing.

Test documentation updated?

@tmat tmat requested a review from a team as a code owner April 11, 2018 17:54
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Apr 11, 2018

@dotnet/roslyn-compiler Please review.

@tmat tmat added this to the 15.7 milestone Apr 11, 2018
@tmat
Copy link
Copy Markdown
Member Author

tmat commented Apr 11, 2018

@nguerrera PTAL

Declare that target InitializeSourceRootMappedPaths that populates MappedPaths metadata on SourceRoot items is available.
-->
<PropertyGroup>
<SourceRootMappedPathsFeatureSupported>true</SourceRootMappedPathsFeatureSupported>
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.

Do you want to condition on this property not already being set?

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.

Agree

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Why? This should be true if the target is present. It isn't meant to be overridable by the project.

Copy link
Copy Markdown
Member Author

@tmat tmat Apr 11, 2018

Choose a reason for hiding this comment

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

Ideally msbuild would have TargetExists function that one can pass target name to. This is just a workaround for lack of such feature.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

@agocke agocke left a comment

Choose a reason for hiding this comment

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

LGTM

@tmat
Copy link
Copy Markdown
Member Author

tmat commented Apr 12, 2018

Approved by Barry. Merging.

@tmat tmat merged commit 14d9500 into dotnet:master Apr 12, 2018
@tmat tmat deleted the SourceRootMappedPaths branch April 12, 2018 00:41
tmat added a commit to tmat/roslyn that referenced this pull request Apr 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants