-
Notifications
You must be signed in to change notification settings - Fork 26
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
Added WiX Toolset v4 and newer support #295
base: main
Are you sure you want to change the base?
Conversation
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.
Thank you so much for creating and sharing this PR. It is great work.
The automatic upgrade is a nice workaround to existing v3 WXS files. I would have added a separate subcommand: cargo wix upgrade
or cargo wix migrate
and have it be a separate "step". Having the upgrade be a separate command and step would allow users to debug failures and manually resolve issues. Upgrading only needs to be done once and only if v3 files already existed.
A separate subcommand also provides a nice separation of concern.
New users/projects do not need to upgrade. The ultimate solution is have two separate mustache files, one for v3 and one for v4. Then, roll over to v4 as the default toolset for new/existing projects.
That said, this implementation is a very nice "in-between" solution and iteration. I would like to see a --no-upgrade
flag/feature that disables the check and upgrade if desired. If the upgrading is moved to a separate subcommand, then this flag is not needed.
I wonder if it makes sense to also have the upgrading and v4 behind a feature flag?
I have not had a chance to test this PR locally, but I wanted to share some initial thoughts and provide some feedback. Excellent work!
Sure, actually as-is all of this is opt-in, so without the flag it defaults to no-check and no-upgrading same with the toolset default.
Sure, I don't mind splitting this up into a different subcommand, it was just easier to bake it into create because create has all of the source file plumbing. Thanks for the review, I'll address your comments as soon as I can! |
@volks73 I added a "setup" subcommand. Here is what the cli help would look like for
and for create:
|
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 like the creation and organization of a toolset
module to put all of the toolset, v3, v4, etc., handling code into a separate module. Keeps it nice and clean.
Thank you for breaking out the upgrade-related activity into a separate subcommand. I like the additional utilities and functionality you have added within this subcommand.
I would like the subcommand to be renamed from setup
to either: (1) upgrade
or (2) migrate
. The setup
subcommand "conflicts" with the cargo wix init
command. As a first time user, am I supposed to use the cargo wix init
or cargo wix setup
command? Based on this question, I would lean towards the migrate
subcommand to avoid confusion.
WiX v4 extensions are stored in the .wxs
folder or are these installed globally? I have to read up on WiX v4 extension system.
Extensions end up being stored in a Sure I can rename the command. I had a hard time figuring out how to name it because the restore logic is actually handy between wix versions as the normal wix extension installer won't pick the right version for its current self it just goes with the latest version. 🤦🏻♂️ |
Starting to work on the new template, here are the baseline things changed by convert. Putting this here for posterity
|
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.
The tests are coming along nicely. You are moving fast! Great work.
@volks73 This test is running into issues, |
fix: bugs found from tests chore: cargo fmt
@juliusl Yes, it appears to be that way. This is probably a bug. |
@volks73 so sanity check question, from your experience are there any wix projects that do this,
Looking around on the web, it seems like that would be really awkward but not impossible? I'm trying to decide how much energy to expend on ensuring that use case works correctly. |
@juliusl No, not that I have seen in my experience. The usual use case: ./main.wxs
./somedir/include_1.wxs
./somedir/include_2.wxs or ./wix/main.wxs
./wix/include_1.wxs
./wix/include_2.wxs
./wix/include_3.wxs or ./wxs/main.wxs
./wxs/includes/1.wxs
./wxs/includes/2.wxs
./wxs/includes/3.wxs I also do not recall receiving a request or issue with that WiX project layout, so if it is becoming too much of a burden, then please skip and we can address in the future. |
@volks73 sweet, I'll put a warning on the screen at least or maybe just panic and say "migrate does not support nested .wxs source files" |
Lol the classic unix vs windows file path conflicts
|
fix: make test `test_project_upgrade_extension_detection` system agnostic
@juliusl Maybe not panic, but gracefully exit with the message. |
fix: linux adds a cd when cd is set on cmd
Addreses #174 and #293
-- Current code coverage stats --
--toolset
disambiguation ininit
command (Ended up going with --schema flag to disambiguate, updatedprint
command as well)sxs
mode, need to change the wxs source enumeration to reflect the changes