Fix PublishDepsFilePath setting by swaroop-sridhar · Pull Request #11462 · dotnet/sdk

swaroop-sridhar · 2020-04-28T07:01:22Z

Restore the semantics of PublishDepsFilePath to be location where the deps.json resides when published.
The PublishDepsFilePath property is used by external projects (ex: asp.net) which rely on this definition.

PublishDepsFilePath is empty (by default) for PublishSingleFile, since the deps.json file is embedde within the single-file bundle.

Add a test case for this property.

Restore the semantics of PublishDepsFilePath to be location where the deps.json resides when published. The PublishDepsFilePath property is used by external projects (ex: asp.net) which rely on this definition. PublishDepsFilePath is empty (by default) for PublishSingleFile, since the deps.json file is embedde within the single-file bundle. Add a test case for this property.

dougbu

Works for me but this is @dsplaisted's parade

dsplaisted · 2020-04-28T15:52:28Z

What would the guidance be for consumers of this property if they are run for a single file app? This change looks like it fixes ASP.NET because it reverts to the previous behavior for a non-single-file app. But if there's a target that wants to rewrite the deps file and it should work for single-file apps, what are they supposed to do?

dougbu · 2020-04-28T18:57:02Z

Hmm, @dsplaisted would it work to restore the previous (never-empty) values of $(PublishDepsFilePath) i.e. eliminate $(_DepsFilePath)?

swaroop-sridhar · 2020-04-28T19:19:42Z

What would the guidance be for consumers of this property if they are run for a single file app? This change looks like it fixes ASP.NET because it reverts to the previous behavior for a non-single-file app. But if there's a target that wants to rewrite the deps file and it should work for single-file apps, what are they supposed to do?

If the meaning of "PublishDepsFilePath" is really the "actual published location" of the deps file, there's no such location in single-file bundle -- because the deps.json is embedded within the single-file bundle. Therefore, this property is unset for single-file apps. This behavior is similar to other location semantics in single-file apps. For example, Assembly.Location returns the empty string for any embedded assembly.

If a target wants to modify the deps file, it needs to insert itself between GeneratePublishDependencyFile and GenerateSingleFileBundle targets. To help with this, we could expose the _DepsFilePath target as IntermediateDepsFilePath which can be used in single-file or non-single-file cases.

swaroop-sridhar · 2020-04-28T19:20:49Z

Hmm, @dsplaisted would it work to restore the previous (never-empty) values of $(PublishDepsFilePath) i.e. eliminate $(_DepsFilePath)?

I don't think this is a good idea -- because $(PublishDepsFilePath) will change meaning from
"published deps file path" to "intermediate deps file path" for single-file apps.

swaroop-sridhar · 2020-04-28T23:14:13Z

@dsplaisted : Made the _DepsFilePath property public, as IntermediateDepsFilePath.

dougbu · 2020-04-28T23:15:05Z

From my perspective, the current meaning of $(PublishDepsFilePath) is "location of the .deps file after the GeneratePublishDependencyFile target runs". I don't think naming consistency across msbuild properties (aside from the leading underscore) is a good thing to aim for when dealing with legacy. That ship has sailed. And, a name like $(IntermediateDepsFilePath) is a strange one if you want to encourage developers to update a file before it's bundled and copied to the final publish or pack location.

I vote against requiring the world to use a new property instead of $(PublishDepsFilePath).

swaroop-sridhar · 2020-04-28T23:23:41Z

@dougbu we only need to use IntermediateDepsFilePath if updating the deps.json file for single-file apps.

I think using IntermediateDepsFilePath will make the fact that the deps.file needs to be updated in the "intermediate" stage before bundling into a single-exe, more evident. But I'm not too stuck on the name to use.

I'm wary of keeping PublishDepsFilePath pointing to the obj\...\app.deps.json file for single-file apps, because consumers may try to change it post publish (since the file still exists) and expect it to work.

dougbu · 2020-04-28T23:47:44Z

@swaroop-sridhar understand your wariness. I lean toward avoiding breaking changes and you're leaning toward easily explainable general rules. The reason for my leaning is mainly that nobody reads the general rules. But my thoughts on single-file publish are theoretical -- I don't use that infrastructure. If $(PublishDepsFilePath) works like it used to for the non-single-file case, at least aspnetcore will be good.

And, if this also fixes the new NU5118 warnings, all the better.

swaroop-sridhar · 2020-04-29T01:02:31Z

My suggestion is to take this fix, and unblock the SDK update.
We can then discuss what PublishDepsFilePath should look like on single-file apps.

ViktorHofer · 2020-04-29T18:20:00Z

Can we please reach consensus here as this currently blocks both aspnetcore and arcade updating to a newer sdk. For aspnetcore this is even on the critical path for preview4.

swaroop-sridhar · 2020-04-29T19:02:38Z

@dsplaisted please review.

dsplaisted · 2020-04-29T18:58:31Z


    <PropertyGroup>
-      <PublishDepsFilePath Condition=" '$(PublishDepsFilePath)' == ''">$(IntermediateOutputPath)$(ProjectDepsFileName)</PublishDepsFilePath>
+      <!-- _DepsPath is the location where the deps.json file is originally created


Looks like this comment is out of date with the current code. I don't see a _DepsPath property.

dsplaisted · 2020-04-29T19:05:12Z

+            var publishDepsFilePath = GetPropertyValue(projectPath, targetFramework, "PublishDepsFilePath");
+            var expectedDepsFilePath = $"{publishDir}{projectDepsFileName}";
+
+            publishDepsFilePath.Equals(expectedDepsFilePath).Should().BeTrue();


I think it's better to write the assertion like this:

publishDepsFilePath.Should().Be(expectedDepsFilePath)

That gives more diagnosable failure messages.

(This also applies to the other assertions)

dsplaisted · 2020-04-29T19:06:58Z

+        {
+            var testProject = SetupProject(singleFile: false);
+            var testAsset = _testAssetsManager.CreateTestProject(testProject);
+            var restoreCommand = new RestoreCommand(Log, testAsset.Path, testProject.Name);


I think you can remove the explicit restore commands. The GetValuesCommand should restore by default for you.

dsplaisted · 2020-04-29T19:08:22Z

Looks good, we can take this change. I did not think it was blocking for taking an updated SDK, I thought that there was a workaround.

dougbu · 2020-04-29T19:21:15Z

I thought that there was a workaround.

It's possible to create a workaround but I don't have one yet. And, if this is also at the root of the new NU5118 warnings, the only workaround there is to suppress the warnings i.e. to sweep things under the rug.

dougbu · 2020-04-29T23:47:10Z

Thanks @swaroop-sridhar and @dsplaisted. About how much time will likely pass between this merge and a new SDK version that dotnet-install.ps1 / dotnet-install.sh can install?

dougbu · 2020-04-29T23:50:44Z

Better question: Is completion of https://dev.azure.com/dnceng/internal/_build/results?buildId=624059&view=results plus ~15 minutes enough for this fix to be available?

dsplaisted · 2020-04-30T00:16:59Z

@dougbu, no, once that build completes then it needs to get inserted into the installer repo (which is automated, though it sometimes needs manual intervention). Here's the most recent PR that updated the SDK in installer: dotnet/installer#7351

dougbu · 2020-04-30T00:25:20Z

Ah, then https://dev.azure.com/dnceng/internal/_build/results?buildId=624059 completion (soon) + ~15 minutes for Maestro++ machinations + ~2 hours for a dotnet/installer PR and a https://dev.azure.com/dnceng/internal/_build?definitionId=286 run + another ~15 minutes i.e. likely 2.5 hours from now (best case)?

dougbu · 2020-04-30T00:28:09Z

Never mind, the current pipeline just failed in the (Arcade) Signing validation step: https://dev.azure.com/dnceng/internal/_build/results?buildId=624059&view=logs&j=0a6c679f-72be-5867-422e-acb741a068a3&t=d8e44ddf-8ea2-5518-25e7-4300471a271a&l=20 More waiting ☹️

dougbu · 2020-04-30T00:31:08Z

(@dsplaisted @swaroop-sridhar the 'master' branch of dotnet/sdk started failing after an update from microsoft/msbuild. I don't see this error before https://dev.azure.com/dnceng/internal/_build/results?buildId=622370&view=logs&j=0a6c679f-72be-5867-422e-acb741a068a3&t=d8e44ddf-8ea2-5518-25e7-4300471a271a&l=20)

swaroop-sridhar mentioned this pull request Apr 28, 2020

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

Merged

swaroop-sridhar requested a review from dsplaisted April 28, 2020 07:01

dougbu reviewed Apr 28, 2020

View reviewed changes

swaroop-sridhar mentioned this pull request Apr 28, 2020

Upgrade Arcade to a 5.0 SDK dotnet/arcade#5357

Merged

Rename _DepsFilePath to IntermediateDepsFilePath

d7a8b60

dsplaisted approved these changes Apr 29, 2020

View reviewed changes

Address feedback by @dsplaisted

c48cf9f

swaroop-sridhar merged commit c09668e into dotnet:master Apr 29, 2020

dsplaisted mentioned this pull request May 1, 2020

Correct $(IntermediateDepsFilePath) logic #11527

Closed

dsplaisted mentioned this pull request Sep 10, 2020

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

Closed

Conversation

swaroop-sridhar commented Apr 28, 2020

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

dsplaisted commented Apr 28, 2020

Uh oh!

dougbu commented Apr 28, 2020

Uh oh!

swaroop-sridhar commented Apr 28, 2020

Uh oh!

swaroop-sridhar commented Apr 28, 2020

Uh oh!

swaroop-sridhar commented Apr 28, 2020

Uh oh!

dougbu commented Apr 28, 2020

Uh oh!

swaroop-sridhar commented Apr 28, 2020

Uh oh!

dougbu commented Apr 28, 2020

Uh oh!

swaroop-sridhar commented Apr 29, 2020

Uh oh!

ViktorHofer commented Apr 29, 2020

Uh oh!

swaroop-sridhar commented Apr 29, 2020

Uh oh!

dsplaisted Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted Apr 29, 2020

Choose a reason for hiding this comment

Uh oh!

dsplaisted commented Apr 29, 2020

Uh oh!

dougbu commented Apr 29, 2020

Uh oh!

dougbu commented Apr 29, 2020

Uh oh!

dougbu commented Apr 29, 2020

Uh oh!

dsplaisted commented Apr 30, 2020

Uh oh!

dougbu commented Apr 30, 2020

Uh oh!

dougbu commented Apr 30, 2020

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dougbu commented Apr 30, 2020 •

edited

Loading