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 not passing -arch #129

Merged
merged 1 commit into from
Nov 19, 2020
Merged

Fix not passing -arch #129

merged 1 commit into from
Nov 19, 2020

Conversation

roblabla
Copy link
Contributor

Looks like when I did the last rebase in #113 (over the reformatting commit), I forgot to re-add the -arch argument. Oops. This caused the Wix to always build 32-bit MSIs. This fixes it.

@volks73
Copy link
Owner

volks73 commented Nov 19, 2020

We may need to review all of the tests and see how this slipped through. I would think one of the integration tests would have been able to pick up that 32-bit MSIs were being created instead of 64-bit MSI when requested, but this would be for a different issue/PR.

I was going to merge, but there are some failing tests.

@roblabla
Copy link
Contributor Author

roblabla commented Nov 19, 2020

So, the problem boils down to the generated MSI not being actually tested/installed. I think it might be a good idea to add a test that actually runs the MSI through msiexec, and checks that it gets installed properly, and at the right locations. So for a 64-bit executable, it should check that it gets installed in C:\Program Files, while on 32-bit it goes in C:\Program Files (x86).

What's nasty about this bug is that it actually generates a somewhat working MSI. It would just install the wrong things in the wrong places!

I think I fixed the failing test (wix doesn't understand -arch=x64 argument, have to split it in two).

@volks73
Copy link
Owner

volks73 commented Nov 19, 2020

Thanks for fixing the tests.

I think we could move the creation of the "build" Command type to a method within the Execution struct that returns the cmd ready to execute. Then, a unit test could be added to make sure the flags, options, values, etc. that are passed to candle and light is correct. This would at least prevent the -arch being dropped.

I do think at least one test that runs an install is needed, just as a sanity check. I will create an issue to discuss/track.

@volks73 volks73 merged commit df36cfd into volks73:main Nov 19, 2020
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.

None yet

2 participants