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

Publish FileAssociation Icons correctly in Single-File ClickOnce publish #6578

Merged
merged 1 commit into from
Jun 22, 2021

Conversation

sujitnayak
Copy link
Contributor

@sujitnayak sujitnayak commented Jun 17, 2021

Ensure file association icons files get published as loose files in Single-File publish for ClickOnce

Fixes #1340931

Context

ClickOnce applications can associate file extension and icons with the application. In this scenario, the icon files should be published. When published in Single-File mode, we fail to publish the icon files.

Changes Made

In Single-File publishing, ClickOnce only publishes the SF Exe and any uncompressed file that cannot go into the bundle. The ico files get ignored. The fix made now ensure that ico files in FileAssociation item list are also published as uncompressed file. These file need be be outside of the bundle so that the ClickOnce installer and runtime can see them.

Testing

CTI tested all configurations (Portable/x64/x86 across FDD/SCD and SF modes).

Notes

@John-Hart John-Hart requested a review from ning51 June 17, 2021 02:25
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.

Is this the one y'all would like to have for 16.11, or is 17.0 (our current main branch) ok?


<!-- Include file association icons from Content as loose files -->
<_FileAssociationIcons Include="%(FileAssociation.DefaultIcon)"/>
<_ClickOnceFiles Include="@(ContentWithTargetPath)" Condition="'%(Identity)'=='@(_FileAssociationIcons)'"/>
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work if there's more than one FileAssociation.DefaultIcon. Have you tested that case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, I had not. I just tried it and it seems to work.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this the one y'all would like to have for 16.11, or is 17.0 (our current main branch) ok?

Current plan is to fix this for Dev17. We haven't had customers report this corner case failure. If we need to get this into 16.11 at this point, I suppose we need to go through shiproom for approval?

Copy link
Member

@rainersigwald rainersigwald Jun 18, 2021

Choose a reason for hiding this comment

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

I didn't understand how the batching would work as written and I spent a bit of time to build my understanding.

This expression has an unqualified metadata reference %(Identity), so it applies the batching rules to all item lists in the expression. Since _FileAssociationIcons and ContentWithTargetPath both have the .ico files as entries, this works fine.

(Someday I will internalize this, but it's not today. And it wasn't two years ago either: #4429)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks makes sense. Essentially I want to iterate through items in ContentWithTargetPath item list and add the item to _ClickOnceFiles only if it exists in _FileAssociationIcons item list.
Is there a better way to express this in msbuild?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rainersigwald
Just want to make sure the change I have is the correct way to express the intent. If so, I will go ahead with the merge.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, sorry. Yes, this is fine.


<!-- Include file association icons from Content as loose files -->
<_FileAssociationIcons Include="%(FileAssociation.DefaultIcon)"/>
<_ClickOnceFiles Include="@(ContentWithTargetPath)" Condition="'%(Identity)'=='@(_FileAssociationIcons)'"/>

Choose a reason for hiding this comment

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

Do we need to do something similar for other content files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only content files that need special handling are the file association icons afaik. I don't see any other place where ad-hoc content files are added indirectly as publishing content.

@Forgind Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jun 21, 2021
@sujitnayak sujitnayak merged commit 52c4151 into dotnet:main Jun 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants