/DotNetTools

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stop using MSBuildProjectExtensionsPath in dotnet-watch and user-secrets #347

Merged
merged 2 commits into from Sep 26, 2017

Conversation

Reviewers

@bricelam bricelam

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@natemcmaster @bricelam @dnfclas
@natemcmaster
Member

natemcmaster commented Sep 22, 2017

Resolves #244

Use CustomAfterMicrosoftCommonTargets instead of MSBuildProjectExtensionsPath.

  • No more need to write to obj/$(Project).g.dotnetwatch.targets
  • Works on project that have changed default file locations via BaseIntermediateOutputPath

Simplify DotNetWatch targets

  • Condense to one targets file
  • Simplify dependency chain of targets
  • Build project references in a parallel

@natemcmaster natemcmaster requested a review from bricelam Sep 22, 2017

@dnfclas dnfclas added the cla-already-signed label Sep 22, 2017

src/Microsoft.DotNet.Watcher.Tools/Internal/MsBuildFileSetFactory.cs
"/t:" + TargetName,
"/p:DotNetWatchBuild=true", // extensibility point for users
"/p:DesignTimeBuild=true", // don't do expensive things
"/p:CustomAfterMicrosoftCommonTargets=" + watchTargetsFile,

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Sep 25, 2017

Member

You can't specify multiple targets to include in this property, can you? This prevents projects from using this extensibility point.
Copy URL
@bricelam

bricelam Sep 25, 2017

Member

You can't specify multiple targets to include in this property, can you? This prevents projects from using this extensibility point.

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Sep 26, 2017

Member

Yes, this would override the project's setting. From what I've seen, this property is rarely used, much less than BaseIntermediateOutputPath. Even when used, it seems unlikely to change the list of files in Compile/EmbeddedResource/ProjectReference.

Another option is to write the targets file to MSBuildUserExtensionsPath, though, I have reservations about adding files to machine state, and this wouldn't work across major version changes in MSBuild.
Copy URL
@natemcmaster

natemcmaster Sep 26, 2017

Member

Yes, this would override the project's setting. From what I've seen, this property is rarely used, much less than BaseIntermediateOutputPath. Even when used, it seems unlikely to change the list of files in Compile/EmbeddedResource/ProjectReference.

Another option is to write the targets file to MSBuildUserExtensionsPath, though, I have reservations about adding files to machine state, and this wouldn't work across major version changes in MSBuild.

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Sep 26, 2017

Member

Sounds good. I just wanted to make sure you considered the tradeoffs.
Copy URL
@bricelam

bricelam Sep 26, 2017

Member

Sounds good. I just wanted to make sure you considered the tradeoffs.

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Sep 26, 2017

Member

Btw, before I merge this, are you aware of any other extensibility points I should consider? These are the ones I've looked at:

  • MSBuildProjectExtensionsPath
  • MSBuildUserExtensionsPath
  • CustomAfterMicrosoftCommonTargets
  • CustomBeforeMicrosoftCommonTargets
  • $(MSBuildProjectFullPath).user
Copy URL
@natemcmaster

natemcmaster Sep 26, 2017

Member

Btw, before I merge this, are you aware of any other extensibility points I should consider? These are the ones I've looked at:

  • MSBuildProjectExtensionsPath
  • MSBuildUserExtensionsPath
  • CustomAfterMicrosoftCommonTargets
  • CustomBeforeMicrosoftCommonTargets
  • $(MSBuildProjectFullPath).user

This comment has been minimized.

Show comment
Hide comment
@bricelam

bricelam Sep 26, 2017

Member

Directory.Build.targets :trollface:
Copy URL
@bricelam

bricelam Sep 26, 2017 •
edited

Member

Directory.Build.targets :trollface:

This comment has been minimized.

Show comment
Hide comment
@natemcmaster

natemcmaster Sep 26, 2017

Member

haha. Going with CustomAfterMicrosoftCommonTargets for now.
Copy URL
@natemcmaster

natemcmaster Sep 26, 2017

Member

haha. Going with CustomAfterMicrosoftCommonTargets for now.

natemcmaster added some commits Sep 22, 2017

@natemcmaster
Simplify the MSBuild targets in dotnet-watch 
Use CustomAfterMicrosoftCommonTargets instead of MSBuildProjectExtensionsPath.
 - No more need to write to obj/$(Project).g.dotnetwatch.targets
 - Works on project that have changed default file locations via BaseIntermediateOutputPath

Simplify DotNetWatch targets
 - Condense to one targets file
 - Simplify dependency chain of targets
 - Build project references in a parallel
3dd029a
@natemcmaster
Use CustomAfterMicrosoftCommonTargets in dotnet-user-secrets
Loading status checks…
f3bb408

@natemcmaster natemcmaster merged commit f3bb408 into aspnet:dev Sep 26, 2017
0 of 2 checks passed

0 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@natemcmaster natemcmaster deleted the natemcmaster:proj-eval branch Sep 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment