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

Fix default file inclusion of .RESW and image files in .NET projects #1389

Merged
merged 12 commits into from Sep 24, 2021

Conversation

rohanp-msft
Copy link
Contributor

@rohanp-msft rohanp-msft commented Sep 9, 2021

The existing solution (added in #771) has an issue: app devs cannot opt out an image or a RESW file from PRI inclusion.

Closes #980

How verified (commit 11)

Setup:

  1. Uninstall the existing VSIX.
  2. Install WindowsAppSDK.Extension.Experimental.vsix from the output of the build pipeline run.
  3. Create a new app using the "WinUI.Desktop.Cs.PackagedApp" template.
  4. Install Microsoft.WindowsAppRuntime.1.0.msix manually.

Verification:
(BA below means Build Action)

  1. Copy paste a RESW file, a .PNG file, and a .JPG file into the folder containing the .CSPROJ file. Build and run. Examine PRI. Examine bin placing.
  2. Opt-out the RESW file and the PNG file. Build and run. Examine PRI. Examine bin placing.
  3. Opt-in the files that were opted out. Build and run. Examine PRI. Examine bin placing.
  4. Restart VS. Rebuild and run. Examine PRI.
  5. Opt-out files. Restart VS. Build. Examine PRI.
  6. Set EnableDefaultPRIResourceItems to false. BA of RESW file changed to None. Remove set. BA changes back to PRIResource.
  7. Set EnableDefaultContentItems to false. BA of image file changed to None. Remove set. BA changes back to Content. (BA of image file that was None already remains None and is None after set is removed too.)
  8. Set EnableDefaultItems to false. All image and RESW files disappeared in Solution Explorer (regardless of their BA). Remove set. All files come back.

Note:

  • Issue 1: There is an issue I found for which I'll create a tracking item and link to this PR:
    • Let's say you create a new app. You build and run it. That generates a resources.pri.
    • You then copy-paste some resource files into the project folder. The Build Action of these files will be set correctly by default (which is the point of the change).
    • You then hit F5.
    • The resources.pri in the build output after the F5 Run doesn't have the new resources. I suspect the PRI is not regenerated.
    • If the app is rebuilt, then the PRI's contents are correct.
  • Issue 2: This solution does not work for resource files that are added from outside the project folder. For them, manual action is needed to set the Build Action to PRIResource or Content. I'll create a tracking item for this too and link to this PR.

@rohanp-msft

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@rohanp-msft

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Copy link
Contributor

@huichen123 huichen123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@rohanp-msft rohanp-msft force-pushed the user/rohanp/fix_build_action_issue branch from 6de2bf5 to f94b815 Compare September 10, 2021 00:02
@rohanp-msft

This comment has been minimized.

@rohanp-msft

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

Copy link
Member

@rainersigwald rainersigwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to be sure that things like the introduction of EnableDefaultPRIResourceItems is not unacceptable

I think this is a very reasonable extension of the current pattern. With MSBuild there's always a risk of name collision but this is very specific and doesn't appear to be currently used.

@dsplaisted do you have any concerns from the SDK perspective?

dev/MRTCore/packaging/MrtCore.props Outdated Show resolved Hide resolved
Copy link
Member

@dsplaisted dsplaisted left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The globbing looks good, but it seems like there may be issues with moving the EnableCoreMrtTooling logic to .props files.

A possible way to work around this would be to move the logic that sets EnableCoreMrtTooling back to the .targets, import MrtCore.props unconditionally, and then add a check for EnableCoreMrtTooling to all the ItemGroups in MrtCore.props.

build/NuSpecs/Microsoft.WindowsAppSDK.Foundation.props Outdated Show resolved Hide resolved
@rohanp-msft rohanp-msft force-pushed the user/rohanp/fix_build_action_issue branch from f94b815 to 34e1254 Compare September 19, 2021 05:10
@rohanp-msft

This comment has been minimized.

@azure-pipelines

This comment has been minimized.

@rohanp-msft rohanp-msft force-pushed the user/rohanp/fix_build_action_issue branch from d2d1b85 to ed66a7a Compare September 23, 2021 01:57
@rohanp-msft rohanp-msft force-pushed the user/rohanp/fix_build_action_issue branch from 469a24e to 28c3285 Compare September 24, 2021 00:34
@rohanp-msft
Copy link
Contributor Author

/azp run

@rohanp-msft rohanp-msft enabled auto-merge (squash) September 24, 2021 00:34
@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

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.

Adding an icon to a package with build action None still adds it to the .pri resource
5 participants