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

[PTRun]Fix locking link files for MSI installers. Warning 1946 #37654

Merged
merged 4 commits into from
Feb 27, 2025

Conversation

donlaci
Copy link
Collaborator

@donlaci donlaci commented Feb 26, 2025

Summary of the Pull Request

This PR extends the IpersistFile Load method with parameters to make no change on the loaed item --> to not block other programs (MSI installer) modifying the item.

See: microsoft/WSL#11276 (comment)

The STGM_TRANSACTED parameter is added, which ensures that any changes are buffered. Without this parameter the issue persists, adding this parameter solves the issue.

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

Tesed locally with https://github.com/avjts/powerlauncher-bug

@jaimecbernardo jaimecbernardo changed the title [Runner] fix MSI installer issue. Warning 1946 [PTRun]Fix locking link files for MSI installers. Warning 1946 Feb 26, 2025
@OneBlue
Copy link

OneBlue commented Feb 26, 2025

@donlaci: Unfortunately I'm still able to reproduce the issue with this change

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 26, 2025

@OneBlue
Can you share the lnk file somehow with @donlaci ? (Maybe it has something special.)

@donlaci
When indexing the shortcuts on the desktop by program plugin, do we use this global helper or do we use specific code inside the plugin? 🤔

@htcfreek
Copy link
Collaborator

htcfreek commented Feb 27, 2025

@donlaci
Is it true that we never release the pointer to the "persistent file" object? See here.

In the docs example it's done.

@donlaci
Copy link
Collaborator Author

donlaci commented Feb 27, 2025

@OneBlue Strange, on my system without the changes the issue comes after 1-2 installation processes, with the changes I cannot reproduce it after 15++ installation processes. Is there at least any improvement on your side?
@htcfreek The Marshal.ReleaseComObject(link); gets called to release the memory.

@jaimecbernardo
Copy link
Collaborator

@OneBlue , do you have access to Dart builds? Mind trying out with a release-like PowerToys installer? https://microsoft.visualstudio.com/Dart/_build/results?buildId=117134335&view=results
Thank you.

@jaimecbernardo
Copy link
Collaborator

@OneBlue , I gave the fix a try with the binaries from Dart. This fixes it for Laszlo on Windows 11. I gave it a test on Windows 10 and without the fix I can repro the issue everytime. With the fix, I can't repro the issue. Can you please give it another try?
Here's a locally built unsigned installer as well in case you can't download the other one from Dart:
https://github.com/jaimecbernardo/PowerToys/releases/tag/v0.88.17-test-pr37654

If the issue persists, can you please give exact instructions on how you're trying to repro? Thanks, in advance.

@jaimecbernardo
Copy link
Collaborator

@OneBlue, the previous fix broke some functionalities, so we had to refine it. Updated installers for you to test were uploaded to https://github.com/jaimecbernardo/PowerToys/releases/tag/v0.88.17-test-pr37654 as v0.88.18 instead.
Also started a new Dart build on https://microsoft.visualstudio.com/Dart/_build/results?buildId=117204739&view=results

Copy link
Collaborator

@jaimecbernardo jaimecbernardo left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for the fix. Would be good to have some confirmation from more sources, but the code seems to be fixing the issue for me and we still get programs filled from link files.

@OneBlue
Copy link

OneBlue commented Feb 27, 2025

Interesting, let me try again with that installer and see if I still hit the issue.

@OneBlue
Copy link

OneBlue commented Feb 27, 2025

@jaimecbernardo: I confirm that I couldn't reproduce the issue with this installer. I tried multiple times and the MSI warning didn't appear.

Maybe I got the wrong installer last time ?

Either way, this LGTM !

@jaimecbernardo
Copy link
Collaborator

Thanks for the feedback :) I'll merge it and include it in this upcoming version then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants