volks73 / cargo-wix

A cargo subcommand to build Windows installers for rust projects using the WiX Toolset
https://volks73.github.io/cargo-wix
Apache License 2.0
306 stars 25 forks source link

`cargo wix print wxs` produces platform-specific output #219

Closed Gankra closed 1 year ago

Gankra commented 1 year ago

In particular it uses Path/PathBuf heavily when generating paths to write to a file, so \ vs / happens. Now this is basically fine because you're not "supposed" to use cargo-wix on not-windows, as WiX itself is windows only... however nothing fundamentally prevents print/init from working cross-platform. As such, the generated main.wxs should ideally be platform-independent.

I don't know why I didn't catch this earlier, but our CI which checks that it doesn't need to be changed just tripped over this issue (because it's part of the normal test suite which gets run on every platform, so it will always fail unless the output is deterministic across all platforms).

Gankra commented 1 year ago

I'm on the fence over whether we should canonicalize to \ or /... not sure how brittle WiX is to unix style separators (which otherwise work fine in windows).

volks73 commented 1 year ago

I am confused. To clarify, you want to drop usage of Path/PathBuf and use a hard-coded constant of \ for the path separator in the XML output?

Would it be possible to run your MSI tests only on Windows test runners for the CI?

Gankra commented 1 year ago

I am confused. To clarify, you want to drop usage of Path/PathBuf and use a hard-coded constant of \ for the path separator in the XML output?

Yes.

Would it be possible to run your MSI tests only on Windows test runners for the CI?

Not really. The failure I hit was in my unit tests, but the way we're planning to expose this to users of cargo-dist will be running the check on linux-only, so they'd never run it on windows. Many users will also be running cargo dist init (and therefore cargo wix print) on linux (because they don't have a windows dev machine, only windows CI and a trust of their tools to work). In this case we want it to produce the correct result, which requires the linux build of cargo-wix to have reliable "print".

If this is an unreasonable request we can potentially fudge it in cargo-dist with post-processing, but there's some real sketchiness in that (hard for us to reliably distinguish paths from other things).

volks73 commented 1 year ago

I am not opposed to switching to a "constant" (I would prefer a function and even a CLI option to set the path separator as needed).

So, users would define paths at the CLI with UNIX-style paths and then cargo wix would have to convert UNIX-style paths to Windows paths where appropriate in the XML?

Gankra commented 1 year ago

If the user provides the path I think it's fine for us to preserve it verbatim (unless you're aware of WiX not being able to handle unix-style paths even though everything else on Windows does).

However any path segments that WiX adds, like Path::new("$(var.CargoTargetBinDir)").join(name), should just hardcode the backslash (that one is a super dubious use of std::path anyway...)

Gankra commented 1 year ago

Also tbc if a user passed "x/y/z" to cargo-wix right now, using Path/PathBuf on windows doesn't rewrite those. It forwards it along. PathBuf::from("x/y/z").join("w") produces "x/y/z\w", so I'm not actually suggesting a behaviour change on windows, just making unix match up.

Gankra commented 1 year ago

Hopefully adequately fixed by #222