Skip to content
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

MSBuild properties for hosted deployment options #26620

Merged
merged 11 commits into from
Aug 5, 2022
Merged

Conversation

guardrex
Copy link
Collaborator

@guardrex guardrex commented Aug 3, 2022

Fixes #26619

@yugabe ... Are you free to take a look 👀 at this?

btw -- You'll see three copies of the section. You only need to look at the first because they're identical. We duplicate them to cover different release versions in the same article.

The new section for the WASM deployment doc seeks to address both the general scenario for any given MSBuild property that's applied to both projects of a hosted WASM solution and specifically for your use case scenario (framework-dependent executable for a specific platform), which I show as the example.

You (and Javier) didn't state that there was a limitation on the self-contained deployment option (as false) to the project file. You did state that ...

Disabling SCD from the CLI breaks the build altogether.

... but you may have meant by way of --no-self-contained, which seems like it would 💥 based on Javier's explanation. I came away with the impression that SelfContained can also be stated as an MSBuild option (/p:) in a similar fashion to the RID to meet your needs ...

dotnet publish /p:RuntimeIdentifier={RID} /p:SelfContained=false

However, let me know if you tried that in your testing and it failed. If so, then we'll need to figure that out as an additional complication here. I may need to work on this a bit further with Javier's help. I'm trying to avoid pinging him due to the high .NET 7 product unit workload at this time. If this all looks good, then we can proceed. I'll take further reader feedback on it.

@guardrex guardrex self-assigned this Aug 3, 2022
@yugabe
Copy link

yugabe commented Aug 3, 2022

Thank you @guardrex for taking a look.

Firstly, the wording of the added section seems very comprehensible to me. There is a part of a sentence missing in the second paragraph or confusing wording though:

Consider the following example, where the developer plans to publish a hosted WebAssembly app as both a framework-dependent executable for a specific platform (not self-contained).

I think it should be either "both a framework-dependent executable and for a specific platform" or just remove the "both" (or I'm missing something).

I tried removing setting the SelfContained property in the Server project and passing it MSBuild-style as you suggested. Unfortunately, the error resurfaces that way:

C:\Program Files\dotnet\packs\Microsoft.NET.Runtime.WebAssembly.Sdk\7.0.0-preview.6.22324.4\Sdk\WasmApp.targets(190,5): error : $(MicrosoftNetCoreAppRuntimePackDir)='', and cannot find %(ResolvedRuntimePack.PackageDirectory)=. One of these need to be set to a valid path [C:\...\BlazorNoScd\Client\BlazorNoScd.Client.csproj]

I am very curious of this issue, so I tried modifying my CI build. On Ubuntu, this error breaks the build:

/opt/hostedtoolcache/dotnet/sdk/7.0.100-preview.6.22352.1/Sdks/Microsoft.NET.ILLink.Tasks/build/Microsoft.NET.ILLink.targets(220,5): error NETSDK1102: Optimizing assemblies for size is not supported for the selected publish configuration. Please ensure that you are publishing a self-contained app. [/home/vsts/work/1/s/src/Client/BlazorNoScd.Client.csproj]
##[error]Error: The process '/opt/hostedtoolcache/dotnet/dotnet' failed with exit code 1

The issue itself seems very weirdly specific. With the goal of having a plaftorm-specific build that is not self-contained:

  • Publishing via Visual Studio publish profile to a folder works without any issues. I assume it would work by calling MSBuild directly if passing the profile via parameter. The publish profile also contains the project setting <SelfContained>false</SelfContained>.
  • When using dotnet publish and passing both an RID and either --self-contained false or --no-self-contained, the publish breaks with the above errors.
  • Not passing the SCD flags directly to the CLI while at the same time not setting the project property results in a warning and falls back to being a self-contained build because RID is specified.

I'm not sure the reasoning of the proposed doc change is totally correct because of the behavior of the build. Seemingly the only combination that works is setting up <SelfContained>false</SelfContained> in the project file (or via publish profiles if using MSBuild/Visual Studio) AND not passing the -r {RID} flag to the CLI.

The wording when building on Ubuntu is also different than what's happening locally on Windows. The Windows error doesn't even hint at the problem, it seems that two path variables are empty that probably shouldn't be at that point in the build. I didn't test it on other machines, so it might be worth checking out if someone's trying to tackle the issue.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 3, 2022

Right ... sorry about that sentence. Yes, I had it structured differently right before one of the latter commits and didn't edit the whole sentence properly. I'll fix that up. ... Oh, I think I already fixed that earlier. Last commit has that fixed.

I'm not sure the reasoning of the proposed doc change is totally correct

Indeed, that's why I was asking. I didn't try to test the scenarios because Javier's explanation seemed to indicate that any MSBuild property misbehaving through the option could be passed via the MSBuild property option ... and that's apparently not the case. It would have been simpler to document.

I'm going OOF for a bit, but I'll be back in a few hours to work further on this. I might go ahead with a round of local testing myself just to see the same behaviors that you're seeing. I'll ping u back later today or on Thursday. Thanks for looking and helping with the coverage on this scenario.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

I ran a series of tests, and I concur with (assuming the RID is passed as the MSBuild property (/p:) ...

  • <SelfContained>false</SelfContained> must be in the Server project's project file for the publishing scenario to work.

  • Passing either the option (--no-self-contained) or the MSBuild property /p:SelfContained=false FAILS to publish output with an error that states ...

    error NETSDK1102: Optimizing assemblies for size is not supported for the selected publish configuration. P
    lease ensure that you are publishing a self-contained app.

    ... for the Client project.

Therefore ...

  • Javier's remark doesn't completely capture what's going on with options/properties in the build pipeline. We can't merely and broadly apply the reasoning that ...

    The issue is that some of the flags passed to dotnet publish don't work well with Blazor apps because they apply to all projects unconditionally, so those settings must be specified using the MSBuild property ...

    That might be correct for the RID, but that reasoning doesn't apply to the self-contained aspect.

    Even if <SelfContained>false</SelfContained> is added to the Client project's project file and the Server app is published with /p:SelfContained=false, it fails to publish with the same error.

  • Because I don't know why the build is breaking without <SelfContained> in the Server project, I'll need to recast the section to state explicit instructions without a reason as to why the special configuration is required. After I update the section on the PR, I'll ask Artak and Dan to take a look and advise further before merging the new section.

We agree on the configuration that should be stated in the new section, so I'll work on updating that now. I'll ping you back when it's ready for another look 👀.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

Ok, @yugabe ... try it now.

This says what to do but not WHY, since we don't cleanly know why. Javier's remark about the RID is fine, but it clearly doesn't apply to the self-contained config. Therefore ... imo ... Javier's reasoning can't be stated as a blanket reason for why any given option may fail. In this case, BOTH --no-self-contained AND /p:SelfContained=false fails ... there's some different reason for its behavior. We don't know what that reason is. It's safe to just tell devs what to do and not say why they're doing it.

WRT pinging Artak and Dan, I changed my mind on that. They're 🏃😅 for the .NET 7 release. As long as we have the guidance correct, I think we can roll with it and take further feedback on it.

I'll wait until u provide feedback. Based on changes or not, we'll merge when we're happy with the coverage.

@yugabe
Copy link

yugabe commented Aug 4, 2022

Considering the alternative (having to go deep into the bowels of the CLI, Blazor publishing and MSBuild), I think this will have to do. Maybe one piece of information worth adding is publishing via Visual Studio works by using a publish profile, as it implicitly sets the <SelfContained>false</SelfContained> for the Server project (in the .pubxml file). To simplify, you might just append "using the .NET CLI" (or something to the same effect) to the paragraph and sidestep the issue altogether:

To deploy a hosted Blazor WebAssembly app as a framework-dependent executable for a specific platform (not self-contained):

Also, maybe a direct link to /aspnet/core/host-and-deploy/ would be helpful as well in the closing paragraph if someone stumbles upon this.

On a sidenote, my original issue is essentially resolved, and as I understand you couldn't reproduce my error message, but I wonder where that was coming from. If I got the same error message that you did (Optimizing assemblies for size is not supported for the selected publish configuration. Please ensure that you are publishing a self-contained app.), then I'd say it's a bit misleading.

The reasoning has to be along the lines that the Blazor WASM application can be nothing else than a self-contained app: in the browser, the app won't be able to access any .NET runtime on the host, so it has to come with the app. On the other hand, making the Server (which also includes a different, not WASM-compiled assembly of the Client, because it is referenced; enabling prerendering and such is only possible this way) framework-dependent requires that instance of the Client build be framework-dependent too.

Setting the Client to <SelfContained>false</SelfContained> breaks the WASM build; and setting it to true breaks the Server build (whatever RID is being targeted) if trying to build the Server as framework-dependent.

Most probably because of this, the build breaks at different points, because two Client builds need to be built with different configurations. This behavior is probably undefined, and breaks the build in these cases.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

@yugabe ... I tried to shoehorn the VS pieces into this. I think it's ok. The alternative would've been to split out the VS bits, but that would make it longer. See if the latest round of updates sound good 👂.

@yugabe
Copy link

yugabe commented Aug 4, 2022

I think it's good as is. Thank you for your work.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

Actually, I think I will edit it again ... the sentences are a bit long and complex for machine translation into other languages.

@yugabe
Copy link

yugabe commented Aug 4, 2022

All the hidden things that make it worthwhile... I never would have even considered that an issue, but you're right of course.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

Ok ... I like that first bullet better for the self-contained setup. It will probably translate better this way as a list than in a L...O....N....G sentence.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

IIRC, there was a way to pass things to the dotnet publish command under-the-covers in VS (i.e., setting the RID that way), but I don't recall where that box is in the VS UI. There's a box where you can place /p:RuntimeIdentifier={RID} and it should work without having to set it in the publish profile UI ... do you recall where that is?

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

I thought it was in the Build properties of the project, but I don't see it there.

Naaaah! ... Doesn't matter. I don't know where that box is or if they removed it from VS. Anyway, the guidance on the PR now should be correct.

I'm ready to merge this if you are.

@yugabe
Copy link

yugabe commented Aug 4, 2022

I was under the impression that the publish profile was only picked up by MSBuild and not by the dotnet CLI, but maybe I missed something regarding passing arguments through.

Mixing in publishing with Visual Studio messed up the steps a bit. To be clear, when not using the CLI and only going through the VS dialogs, all's well, no errors whatsoever. This is in conflict with the two bullet points a bit.

If I understand right:

  • Option 1: use VS to publish, set the RID and the Self-Contained flags as you like. It'll work as far as my experiments went. (I don't know what magic they do, but it works as it should.)
  • Option 2: use the CLI. In this case:
    • DON'T pass either the CLI SCD flags (--self-contained false or --no-self-contained), nor the /p:SelfContained=false flag to CLI to disable SCD;
    • DO set the <SelfContained>false</SelfContained> in the Server project;
    • DO set the RID in the project file OR pass /p:RuntimeIdentifier {RID}.

The current structure seems to imply that setting it via publish profile might work with the CLI.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

Yes, I think you're correct that it will need to be split out because we will NOT further go into the details of indicating the publish profile via the CLI, which is possible to do with /p:PublishProfile={PROFILE}. I wish these aspects were simpler to document, but with multiple approaches for multiple tools ... ei ei ei 🤦‍♂️ ... well ... that's what they pay me for! 😆

I'll return to this either later this afternoon or on Friday morning. Perhaps, my .... er ... FOURTH attempt 🙈 will be fruitful.

@yugabe
Copy link

yugabe commented Aug 4, 2022

I'm sorry this turned out to be such a pain, but I guess it's still better to document it properly than fix whatever constellation is causing this under the hood... I wouldn't even know where to start...

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 4, 2022

No worries. Yeah, it will work out in the end. Due to the complexities of multiple tools and multiple approaches with those tools, this is a bit challenging to get just right, but I think I see what needs to be done now to finish it.

WRT the why aspects on RID versus self-contained, I might ask Javier later ... much later ... like EOY or 23Q1 ... to look at this section and see if he wants to make any why remarks. I'll add that to one of my tracking issues now so that I don't forget.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 5, 2022

@yugabe ... I split out the VS pieces, and I decided to add a NOTE on using the publish profile with the dotnet publish command if the dev wants to use a VS publish profile. I tested that approach here, and it worked ✨.

See if I missed anything 🤞.

@yugabe
Copy link

yugabe commented Aug 5, 2022

Ha, yes, I see you couldn't resist to leave that part out 😁

I think this is complete, at least as far as our knowledge permits at the moment. I do "approve" (I'm not sure I should approve in the GitHub PR, I guess someone from the team will do that).

In the meantime, I convinced myself that my reasoning was correct regarding the Client build having to be built for both a self-contained browser-wasm target and a framework-dependent, parameterized RID target being the root cause of the problems. Of course, this is in no way a correct response for the WHY, but it might be a good starting point when this issue resurfaces in the future.

@guardrex
Copy link
Collaborator Author

guardrex commented Aug 5, 2022

Ok, cool. Thanks for your help with this.

Yeah, I'll catch up with Javier later on the WHY and see if he wants to add anything.

No need for additional approval/sign-offs. I just need to take a vote here on it ...

Here ye! Here ye! Motion to publishing the PR is on the floor. All in favor of publishing this PR?
Aye!
The 'ayes' have it. The motion is carried. The PR is approved. 🎉🕺💃🍾

😆

They told me that if I screw up too many PRs that they'll transfer me to Microsoft's Typewriter Maintenance Division! Yikes!

@guardrex guardrex merged commit c5c1775 into main Aug 5, 2022
@guardrex guardrex deleted the guardrex-patch-1 branch August 5, 2022 11:25
@guardrex guardrex mentioned this pull request Jan 3, 2025
68 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Specify MSBuild prop for some CLI options (e.g., RID)
2 participants