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

Raise conversion warning for After and Before attributes #591

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bevanweiss
Copy link
Contributor

Raise conversion warning for After and Before attributes, at least on Sequence and SetProperty elements

Sequence and SetProperty elements


Signed-off-by: Bevan Weiss <[email protected]>
@robmen
Copy link
Member

robmen commented Jan 17, 2025

This fixes issue 8898, correct? If so, please always add the following line as the last part of the PR message. It ties the PR to the issue and auto-closes it so we don't have to remember to do so later. :)


Fixes wixtoolset/issues#ISSUE_NUMBER_HERE

@bevanweiss
Copy link
Contributor Author

This fixes issue 8898, correct? If so, please always add the following line as the last part of the PR message. It ties the PR to the issue and auto-closes it so we don't have to remember to do so later. :)


Fixes wixtoolset/issues#ISSUE_NUMBER_HERE

not quite... from the issue

Expected Result
The value WixQueryOsDirs should have been replaced with Wix4QueryOsDirs_$(sys.BUILDARCHSHORT).

This doesn't appear to be how the converter has previously handled these wix custom actions, and I've kept it similar.
So my PR just raises the warnings for the conversion locations, it doesn't automatically fix them.
Although that should be pretty straight forward to implement if that's the direction we'd want.

These would need to change to have _$(sys.BUILDARCHSHORT) in place of _<PlatformSuffix> for these

{ "ConfigureComPlusUninstall", "Wix4ConfigureComPlusUninstall_<PlatformSuffix>" },

and an update of the attribute values, plus possibly removal of the warning.

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.

2 participants