-
Notifications
You must be signed in to change notification settings - Fork 297
Windows ML: Fix warnings in cs-winui project #546
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
Conversation
But you're - essentially - disabling publishing. I imagine that this affects MSBuild.exe builds because - IIRC - they publish-on-build, but VS builds don't (since VS has an explicit 'Publish' entry point). So through VS, I would imagine that Publishing just doesn't work. There's other samples that appear to have publishing working, could we try and follow their precedent? For example |
Correct, this is intentionally disabling publishing for the reason I mentioned in the PR description (i.e., I wasn't thinking that a developer would want to actually try to publish these samples or use them as a starting point for that since the focus is on using WinML in a particular type of application). I gave this a try, though. In reply to: 3382233199 |
<PublishProtocol>FileSystem</PublishProtocol> | ||
<Platform>x64</Platform> | ||
<RuntimeIdentifier>win-x64</RuntimeIdentifier> | ||
<PublishDir>bin\$(Configuration)\$(TargetFramework)\$(RuntimeIdentifier)\publish\</PublishDir> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite follow what is supposed to be happening. I don't see actually see anything populated under the location below when I build through either VS or msbuild (the folder structure gets created, but no binaries there):
Samples\WindowsML\cs-winui\bin\Release\net8.0-windows10.0.22621.0\win-x64\publish
Publishing through the Visual Studio UX has things go to a different folder by default, which is Samples\WindowsML\cs-winui\bin\win-x64\publish
:

Running dotnet publish /p:Configuration=Release /p:PublishProfile=win-x64
seems to put things under there.
Samples\WindowsML\cs-winui\bin\Release\net8.0-windows10.0.22621.0\win-x64\publish
But then since this is a packaged app, whatever is in that folder won't be runnable anyway as it's not a package. @mschofie, I am probably holding something wrong here?
For a dotnet build, publishing sounds like a great show-case/test, since we can trim and even AOT. It would be lovely if it worked. I'm trying to buddy build at the minute, and am having problems, it looks like PublishProfile needs to be set correctly: <PublishProfile>Properties\PublishProfiles\win-$(Platform).pubxml</PublishProfile> But then, I'm not able to set the Platform, because the .sln doesn't allow x64. |
Sorry, I missed this comment. IIUC, publishing is more-then/orthogonal to packaging, it allows you to control whether you're .Net Framework dependent or not, whether you AOT or not. There's a bunch of options that would be lovely to showcase. The 'dotnet publish' documentation explains a bit more. TBH, given that publishing is broken because the .sln file is AnyCPU-only, then perhaps your original fix is sufficient (which will address the build breaks), and we file a separate issue to get publishing working? |
Filed #551 for tracking that, I'll update it as I get a better understanding of Publish. Let me know if you'd suggest any other changes to this PR. In reply to: 3382652726 |
This reverts commit 340e2fd.
* Fix warnings in cs-winui project * PR feedback (add publish profiles) * Revert "PR feedback (add publish profiles)" This reverts commit 340e2fd. * Remove PublishProfile --------- Co-authored-by: Aditya Rastogi <[email protected]>
* Fix warnings in cs-winui project * PR feedback (add publish profiles) * Revert "PR feedback (add publish profiles)" This reverts commit 340e2fd. * Remove PublishProfile --------- Co-authored-by: Aditya Rastogi <[email protected]>
Description
There are some warnings-as-errors generated in the cs-winui project when building via the script, e.g.,
build.cmd x64 Release WindowsML
(see below). Broadly speaking, they are related to publishing / deployment aspects of the project. This change resolves the warnings by conditioning the PublishProfile property (the sample is more oriented around local F5 on a developer device), and by enabling signing only if the developer has explicitly provided signing-related pieces. I re-built locally and verified that these no longer show up in the script output and further verified that the sample continues to function as expected.Target Release
1.8.2
Checklist
Note that /azp run currently isn't working for this repo.