Closed Gankra closed 10 months ago
Although there are a couple details left to sort out, I'm marking this as ready for review because I'd like feedback on those details, and if the general approach is something you're happy to land.
Things the subsequent commits do:
Hello! Sorry for the delayed response. This is a very busy time of the year for me.
There is no way to disable eula/license auto-detect
Interesting! This is an oversight in the API and feature set, or a long overdue TODO. It appears the current solution is to generate the WXS file and then remove the EULA UI element from within the XML. This is the same solution for removing the sidecar license file installed next to the binary. However, this works against creating ephermeal WXS files and/or configuration from the project's manifest.
There is the -e,--eula
option for the cargo wix init
and cargo wix print
commands, but that is for the user to explicitly define a path to a RTF file instead of generating one from the project license. As noted, if it is not used, then the EULA is automatically generated. There is the -l,--license
option which operates similar to the -e,--eula
option, too.
It should be noted that there is a difference between an EULA and the license, both in practice and in the code. The EULA is not necessarily the same as the source code license. The EULA is what an end-user can do with the application, while the license is what a developer can or must do with the source code. A developer and end-user might be the same. Many projects share the license with the EULA. The actual correct usage is out-of-scope here, but I created this project, I did a high level survey of Rust projects and the license was typically used as an EULA. So, the default implementation was to use the license as the EULA in the MSI.
However, I apparently started to implement the ability to separate the EULA from the license with the -e,--eula
and -l,--license
options but never fully realized the feature.
I have to take a deep dive into the template and code, but I would think the EULA UI page of the MSI could be wrapped in an "optional" block and tied to a --no-eula
flag and a similar --no-license
flag for the sidecar file.
There is no explicit Cargo.toml config for eula/license
- Cargo's own license_file is weird, it's intended to be instead of license, for situations where your license is Too Weird For SPDX, so having our own dedicated field is desirable)
Yes, the weirdness with the SPDX and having custom licenses was the original motivator for the -e,--eula
and -l,--license
options.
The logic for licenses/eulas is vaguely duplicated, and requires several different locations to agree
This is probably due to the half-implemented separation of between EULAs and licenses.
All-in-all, it looks like the EULA and License management features could use some more attention and refinement. I am open merging any PRs related to this work.
Did all the discussed refactors (just made stored_path a module, not a crate).
@volks73 how would you feel about disabling the "stdout" mode of cargo wix print wxs
(effectively making the output path a required argument). I'm really struggling to rationalize how that's supposed to work correctly with us generating licenses and needing to emit relative paths to things.
(Current status of the PR has it as "still a thing you can ask for, but the output is totally broken")
I'm really struggling to rationalize how that's supposed to work correctly with us generating licenses and needing to emit relative paths to things.
@Gankra The cargo wix print wxs
should not be generating any other output other than a WXS, a.k.a. XML, file. I am missing the issue associated with printing to STDOUT. Requiring the output argument be a path to a file would be the equivalent to redirecting output to file, correct? E.g., cargo wix print wxs > somefile.wxs
is equivalent to cargo wix print wxs somefile.wxs
. But, I admit I have still not done a detailed review of this PR yet.
The print
subcommand generates a string. If the paths are relative it is relative to the current working directory when the user issued the command. Should absolute paths be used instead, or the option/flag for absolute paths?
I was thinking the cargo wix print wxs | cargo wix
workflow was going to be "a thing" for CI and ephemeral builds (https://github.com/volks73/cargo-wix/issues/88), i.e. no artifacts or WXS file would be needed to be included in the source code. Reading a WXS file from STDIN is currently not implemented for the cargo wix
command, but it was something I was thinking about as a feature.
My original thinking was that: cargo wix init
is equivalent to:
cargo wix print MIT > ./wix/license.rtf
cargo wix print WXS > ./wix/main.wxs
Would a cargo wix print --working-dir=./example/path WXS
be useful and resolve the relative path issue? All paths in the printed template to STDOUT would be relative to the working directory that was passed. My desire for consistency and symmetry is kicking in with having the WXS template be "special" and require an argument while others do not.
Thanks for the reply, I think that shook loose a weird assumption I had stuck in my craw (I had convinced myself the paths should be output-dir-relative, rather than purely input-dir-relative). I think I can finish this quickly now.
@volks73 i believe this PR is complete/landable now. cargo-dist can now generate all the subfiles and stuff for people who want the eula functionality:
Downstream I'm preparing a patch that integrates this and adds the new defaults to disable license/eula by default, we're hoping to ship a release with these fixes wednesday morning. If you can review+merge+release by then, that would be grand, but I totally understand if not, and can delay or split the release as necessary.
(got the downstream patch ready, in case you're curious to see what library usage of this API is like: https://github.com/axodotdev/cargo-dist/pull/503)
This is an attempt to fix https://github.com/axodotdev/cargo-dist/issues/468
Apologies that it might be a bit over-engineered/complex, I got really lost in this EULA logic, especially with the multiple implementations of things kicking around.
Issues Being Addressed
Solutions
Centralize License Logic
[package.metadata.wix]
Make wxs The License Generation Boss
initialize
, and make it the responsibility ofprint::wxs
.cargo wix print wxs
will still only print the one file, it probably should emit a warning that you have unresolved dependencies.wix::Execution::render_to_string
withwix::Execution::render
which does all required rendering ofmain.wxs/License.rtf
and returns several RenderOutputs ("the path this wants to be written to" plus "the string to write")wix::Execution::run
path that writes to disk/stdout just usewix::Execution::render
Drive-by Changes
license-name is implemented but never used
Although I originally designed all the logic for setting/passing license-name, once I finished refactoring the code, there were no places that required it. Per your own comments in main.wxs.mustache:
There are no places where we ever need the installed name to differ from the source name... kind of. There was one place in the code I found where the
Name
could be set to a non-trivial value, but it appears to be a typo:https://github.com/volks73/cargo-wix/blob/81e49072f951477317fbf1583bd50e513679bba5/src/print/wxs.rs#L536
That code sets the name to
License
instead ofLicense.rtf
-- I'm not sure why you'd ever want the former. And indeed, I got a lot of confusing issues with different parts of the test suite seemingly disagreeing on that behaviour. Presumably this was the result of the license logic existing in several places? I normalized toLicense.rtf
, which meant license-name was always None. Still, I left in the logic for license-name in case it ever becomes relevant again.auto-licenses will be disabled if no output path is specified to write them to
I'm not sure if this is correct, but it was very confusing to do anything else. To make tests work I changed
print::wxs::Execution::default()
to setoutput
towix\main.wxs
.TODO
cargo wix print wxs
stdout mode