Correct `$(IntermediateDepsFilePath)` logic by dougbu · Pull Request #11527 · dotnet/sdk

dougbu · 2020-05-01T18:44:45Z

restore previous behaviour of this target in the non-single-file case
- ensure $(PublishDepsFilePath) exists after GeneratePublishDependencyFile target runs

- restore previous behaviour of this target in the non-single-file case - ensure `$(PublishDepsFilePath)` exists after `GeneratePublishDependencyFile` target runs

swaroop-sridhar · 2020-05-01T19:37:24Z

-      <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == '' And '$(PublishSingleFile)' != 'true'">$(PublishDir)$(ProjectDepsFileName)</PublishDepsFilePath>      
+      <IntermediateDepsFilePath Condition=" '$(PublishDepsFilePath)' == '' And '$(PublishSingleFile)' != 'true'">$(PublishDir)$(ProjectDepsFileName)</IntermediateDepsFilePath>
+      <IntermediateDepsFilePath Condition=" '$(IntermediateDepsFilePath)' == ''">$(IntermediateOutputPath)$(ProjectDepsFileName)</IntermediateDepsFilePath >
+      <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == '' And '$(PublishSingleFile)' != 'true'">$(IntermediateDepsFilePath)</PublishDepsFilePath>      


If the deps.json file is directly written to the publish directory, it should be removed from ResolvedFileToPublish here in the non-single-file case.

However, I'm not sure its a good idea to write to the publish directory -- before CopyFilesToPublishDirectory has run -- because something may be introduced into ResolvedFileToPublish that overwrites this file. Isn't it?

CC: @dsplaisted

swaroop-sridhar

@dougbu, in this case, I think it is better for InjectRequestHandlerOnPublish to run after CopyFilesToPublishDirectory which will copy the deps file from IntermediateDepsFilePath to PublishDepsFilePath.

If this cannot change due to legacy reasons, then please apply the change I've suggested here.

dougbu · 2020-05-01T19:44:14Z

If this cannot change due to legacy reasons

Won't external developers get tripped up by this breaking change too? For the non-single-file case, running after GeneratePublishDependencyFile and using $(PublishDepsFilePath) worked perfectly. But, I defer to you and @dsplaisted. Thoughts?

dougbu · 2020-05-01T19:45:04Z

Oops, whoever I accidentally tagged instead of @dsplaisted, my apologies.

- not needed when value is the same as `$(PublishDepsFilePath)`

dougbu · 2020-05-01T19:57:40Z

Hmm, what's up w/ the pipeline failure? Did a credential just expire?

dougbu · 2020-05-01T19:57:47Z

/azp run dotnet-sdk-public-ci

azure-pipelines · 2020-05-01T19:57:56Z

Azure Pipelines successfully started running 1 pipeline(s).

dougbu · 2020-05-01T19:58:35Z

Oh good, credential problem went away.

dougbu · 2020-05-01T20:06:33Z

🆙📅 to take @swaroop-sridhar's suggestion in case we decide to fix the latest break. Otherwise I'll use $(IntermediateDepsFilePath) in dotnet/aspnetcore#21017. It doesn't feel right to leave an incorrect .deps file in the obj/ directory and that would happen if I moved our fixup after the CopyFilesToPublishDirectory target. (Must say it also doesn't feel right to leave a .deps file in the obj/ directory at all in the non-single-file case.)

dougbu · 2020-05-01T20:19:08Z

@swaroop-sridhar should I merge this change after PR validation completes or wait for word from @dsplaisted on his preferences?

dsplaisted · 2020-05-01T20:25:45Z

So we're now hitting a different break than we fixed in #11462, right?

The ASP.NET targets:

Run after GenerateBuildDependencyFile and GeneratePublishDependencyFile
Expect PublishDepsFilePath to be the path to a deps file that they can rewrite

I think that means to minimize breakage, we would have to:

Have PublishDepsFilePath point to the path in the output folder
Not use ResolvedFileToPublish to copy to the output folder, because then targets such as ASP.NET may run before it's copied there

It's not feasible for us to make that true for single-file apps, so existing targets could still get broken in that case.

Is that all correct? Is that what these changes are trying to achieve?

Is it possible to update the ASP.NET targets to work with the current logic?

swaroop-sridhar · 2020-05-01T20:25:59Z

@@ -987,10 +987,11 @@ Copyright (c) .NET Foundation. All rights reserved.
    <PropertyGroup>
      <!-- IntermediateDepsFilePath is the location where the deps.json file is originally created
           PublishDepsFilePath is the location where the deps.json resides when published 


Can you please update the comments here to say:
In the single-file case, we write deps file to intermediate dir, and it is bundled into the single-file app.
In the non-single-file case, we write deps file directly in the publish directory due to legacy reasons.

swaroop-sridhar · 2020-05-01T20:33:11Z

So we're now hitting a different break than we fixed in #11462, right?

The ASP.NET targets:

Run after GenerateBuildDependencyFile and GeneratePublishDependencyFile

Expect PublishDepsFilePath to be the path to a deps file that they can rewrite

I think that means to minimize breakage, we would have to:

Have PublishDepsFilePath point to the path in the output folder

Not use ResolvedFileToPublish to copy to the output folder, because then targets such as ASP.NET may run before it's copied there

It's not feasible for us to make that true for single-file apps, so existing targets could still get broken in that case.

Is that all correct? Is that what these changes are trying to achieve?

Is it possible to update the ASP.NET targets to work with the current logic?

Yes this is accurate.

As @dougbu has mentioned, it is possible to fix asp.net targets by using IntermediateDepsFilePath instead of PublishDepsFilePath.
Or, by using PublishDepsFilePath after CopyResolvedFilesToPublishDirectory has run.

This change is trying to preserve legacy behavior for asp.net and other customers who may use these targets/properties not publishing for non-single-file.

dougbu · 2020-05-01T20:34:56Z

Can you please update the comments here to say…

I'll do this if we decide to take this fix. It sounds like @dsplaisted is unconvinced😸

dsplaisted · 2020-05-01T21:10:47Z

I'm unconvinced in the sense that I'm not really sure what we should do.

I think @marcpopMSFT and/or @KathleenDollard were looking at having a process where we at least document breaking changes like this.

So, I think we should fix ASP.NET to work with the current logic to unblock ourselves, and have a discussion about what we should do with this break.

dougbu · 2020-05-01T22:14:12Z

🆗 I'll leave this open rather than fixing the comment or the busted tests (PublishDepsFilePathIsSetAsExpectedForNormalApps and It_cleans_before_trimmed_single_file_publish) immediately. We can use this if we decide to make a change and close it otherwise.

dougbu · 2020-05-01T22:38:53Z

Shoot. I wasn't thinking things through. Using $(IntermediateDepsFilePath) won't work because we also want the path to help us find the native DLL to copy. I'll have to switch to running our target after CopyResolvedFilesToPublishDirectory even though I'd rather not leave an incorrect (and, to me, useless) .deps file in the obj/ folder.

swaroop-sridhar · 2020-05-01T22:52:45Z

Shoot. I wasn't thinking things through. Using $(IntermediateDepsFilePath) won't work because we also want the path to help us find the native DLL to copy. I'll have to switch to running our target after CopyResolvedFilesToPublishDirectory even though I'd rather not leave an incorrect (and, to me, useless) .deps file in the obj/ folder.

I think leaving behind an "incorrect" .deps file in obj folderis not a major concern. It is actually typical of post-processing tools to modify outputs between obj and bin directories. For example, R2R takes files in obj directory, modifies them (elsewhere) and copies the modified output to bin directory. So, the bin and obj directories will not always match.

dougbu · 2020-05-01T22:55:42Z

I think leaving behind an "incorrect" .deps file in obj folderis not a major concern.

I understand that reasoning though the breaking change part of it and the utility of the new obj/ file still feels Just Wrong:tm: 😉

dougbu · 2020-05-02T23:13:27Z

/fyi the CopyResolvedFilesToPublishDirectory target doesn't exist and is a typo in a few comments above. Believe CopyFilesToPublishDirectory is correct.

dougbu · 2020-05-11T00:46:01Z

@dsplaisted @swaroop-sridhar @marcpopMSFT @KathleenDollard have you had a chance to discuss whether to revert this breaking change or documented it?

dougbu · 2020-05-30T02:56:13Z

/ping ❔

swaroop-sridhar · 2020-05-30T06:23:25Z

I recommend that we close this PR, and document the change in PublishDepsFilePath semantics.

dougbu · 2020-06-01T19:00:41Z

@swaroop-sridhar nobody goes looking in the regular docs for notifications of breaking changes. Suggest you need an announcement too.

Correct $(IntermediateDepsFilePath) logic

25a676c

- restore previous behaviour of this target in the non-single-file case - ensure `$(PublishDepsFilePath)` exists after `GeneratePublishDependencyFile` target runs

dougbu requested review from dsplaisted and swaroop-sridhar May 1, 2020 18:44

dougbu mentioned this pull request May 1, 2020

Upgrade to more recent .NET SDK dotnet/aspnetcore#21017

Merged

swaroop-sridhar reviewed May 1, 2020

View reviewed changes

Skip $(IntermediateDepsFilePath) in some cases

41fc8f6

- not needed when value is the same as `$(PublishDepsFilePath)`

swaroop-sridhar approved these changes May 1, 2020

View reviewed changes

swaroop-sridhar reviewed May 1, 2020

View reviewed changes

dougbu mentioned this pull request May 1, 2020

Upgrade Arcade to a 5.0 SDK dotnet/arcade#5357

Merged

dougbu closed this Jun 1, 2020

dougbu deleted the dougbu/IntermediateDepsFilePath branch June 1, 2020 19:00

dsplaisted mentioned this pull request Sep 10, 2020

[Placeholder] Change in deps.json impacting single file applications dotnet/docs#20376

Closed

Conversation

dougbu commented May 1, 2020

Uh oh!

swaroop-sridhar May 1, 2020

Choose a reason for hiding this comment

Uh oh!

swaroop-sridhar May 1, 2020

Choose a reason for hiding this comment

Uh oh!

swaroop-sridhar left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu commented May 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

azure-pipelines Bot commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dsplaisted commented May 1, 2020

Uh oh!

swaroop-sridhar May 1, 2020

Choose a reason for hiding this comment

Uh oh!

swaroop-sridhar commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dsplaisted commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

swaroop-sridhar commented May 1, 2020

Uh oh!

dougbu commented May 1, 2020

Uh oh!

dougbu commented May 2, 2020

Uh oh!

dougbu commented May 11, 2020

Uh oh!

dougbu commented May 30, 2020

Uh oh!

swaroop-sridhar commented May 30, 2020

Uh oh!

dougbu commented Jun 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dougbu commented May 1, 2020 •

edited

Loading