Skip to content

Commit

Permalink
feat(eula): refactor eulas and add new config (#247)
Browse files Browse the repository at this point in the history 
* feat(eula): refactor eulas and add new config

This is an attempt to fix axodotdev/cargo-dist#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
----------------------

* There is no way to disable eula/license auto-detect
* 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)
* The logic for licenses/eulas is vaguely duplicated, and requires several different locations to agree
* Using cargo-wix as a library, there's no way to know about Licenses that are supposed to be generated

Solutions
----------

Centralize License Logic
-------------------------

* Centralize all license/eula logic to eula.rs
* Do all license/eula logic all at once with a new Licenses type
* Replace the Eula enum with a License type that has more details resolved
* Add new license/eula configs to `[package.metadata.wix]`

Make wxs The License Generation Boss
------------------------------------

* Remove all eula/license handling from `initialize`, and make it the responsibility of `print::wxs`.
    * Rationale here is that it doesn't make a lot of sense to *only* produce the main.wxs when it has unresolved dependencies that were supposed to be generated.
    * With this approach if you want to know what the main.wxs should be, you also "have" to know about the other files that need to be generated.
    * Caveat: `cargo wix print wxs` will still only print the one file, it probably should emit a warning that you have unresolved dependencies.
* Replace `wix::Execution::render_to_string` with `wix::Execution::render` which does all required rendering of `main.wxs/License.rtf` and returns several RenderOutputs ("the path this wants to be written to" plus "the string to write")
    * This is the interface that cargo-dist will want to use, so it can enumerate/diff/generate everything
* To avoid a bunch of gross complexities/generics, make the `wix::Execution::run` path that writes to disk/stdout just use `wix::Execution::render`
    * This means instead of writing straight to disk/stdout, we write to a String first
    * The overhead should be negligible given how small the files are, plus it makes it easier to test/debug stuff.
    * The alternative would be defining some kind of Output trait with a bunch of associated types and making all the code generic over it... no thanks.

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:

> Change the value for the `Name` attribute in the `File` tag to the desired name for the file when it is installed alongside the `bin` folder in the installation directory. This can be omitted if the desired name is the same as the file name.

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 of `License.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 to `License.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 set `output` to `wix\main.wxs`.

TODO
-------

* [ ] write more doc-comments
* [ ] add new config fields to docs
* [ ] add tests for new config fields
* [ ] restore some warnings that were lost in the refactor
* [ ] fix tests (I think I lost some strip_prefix stuff)
* [ ] do some integration-testing on cargo-dist to make sure this makes sense

* feat(eula): fix up impl

* feat(eula): add tests for new license fields

* feat(eula): more diagnostics and docs

* feat(eula): rename eula.rs to licenses.rs

* feat(eula): simplify field names

* feat(eula): factor out a stored_path module

* feat(eula): assume we're outputting to PACKAGE_DIR/wix/ for sub-templates
  • Loading branch information
@Gankra
Gankra authored Oct 24, 2023
1 parent 5d83334 commit 7a8a599
Show file tree
Hide file tree
Showing 10 changed files with 1,843 additions and 936 deletions.
116 changes: 0 additions & 116 deletions src/eula.rs
View file

This file was deleted.

42 changes: 5 additions & 37 deletions src/initialize.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,10 @@
use camino::Utf8PathBuf;
use cargo_metadata::Package;

use crate::eula::Eula;
use crate::print;
use crate::stored_path::StoredPathBuf;
use crate::Error;
use crate::Result;
use crate::StoredPathBuf;
use crate::LICENSE_FILE_NAME;
use crate::RTF_FILE_EXTENSION;
use crate::WIX;
use crate::WIX_SOURCE_FILE_EXTENSION;
use crate::WIX_SOURCE_FILE_NAME;
Expand Down Expand Up @@ -456,37 +453,7 @@ impl Execution {
info!("Creating the '{}' directory", destination);
fs::create_dir(&destination)?;
}
let (eula_wxs_path, license_wxs_path) = match Eula::new(self.eula.as_deref(), &package)? {
Eula::CommandLine(path) => (Some(path), self.license),
Eula::Manifest(path) => (Some(path), self.license),
Eula::Generate(template) => {
destination.push(LICENSE_FILE_NAME);
destination.set_extension(RTF_FILE_EXTENSION);
if destination.exists() && !self.force {
return Err(Error::already_exists(&destination));
} else {
info!("Generating an EULA");
let mut eula_printer = print::license::Builder::new();
eula_printer
.copyright_holder(self.copyright_holder.as_ref().map(String::as_ref));
eula_printer.copyright_year(self.copyright_year.as_ref().map(String::as_ref));
eula_printer.input(self.input.as_deref().and_then(Path::to_str));
eula_printer.output(Some(destination.as_str()));
eula_printer.package(self.package.as_deref());
eula_printer.build().run(&template)?;
}
destination.pop();
let mut relative = destination
.strip_prefix(&super::package_root(self.input.as_ref())?)?
.to_owned();
relative.push(LICENSE_FILE_NAME);
relative.set_extension(RTF_FILE_EXTENSION);
let stored_relative = StoredPathBuf::from_utf8_path(&relative);
(Some(stored_relative.clone()), Some(stored_relative))
}
Eula::Disabled => (None, self.license),
};
debug!("eula_wxs_path = {:?}", eula_wxs_path);

destination.push(WIX_SOURCE_FILE_NAME);
destination.set_extension(WIX_SOURCE_FILE_EXTENSION);
if destination.exists() && !self.force {
Expand All @@ -502,17 +469,18 @@ impl Execution {
);
wxs_printer.description(self.description.as_ref().map(String::as_ref));
wxs_printer.dialog(self.dialog.as_deref().map(|s| s.as_str()));
wxs_printer.eula(eula_wxs_path.as_deref().map(|s| s.as_str()));
wxs_printer.eula(self.eula.as_deref().map(|p| p.as_str()));
wxs_printer.help_url(self.help_url.as_ref().map(String::as_ref));
wxs_printer.input(self.input.as_deref().and_then(Path::to_str));
wxs_printer.license(license_wxs_path.as_ref().map(|s| s.as_str()));
wxs_printer.license(self.license.as_deref().map(|p| p.as_str()));
wxs_printer.manufacturer(self.manufacturer.as_ref().map(String::as_ref));
wxs_printer.output(Some(destination.as_str()));
wxs_printer.package(self.package.as_deref());
wxs_printer.path_guid(self.path_guid.as_ref().map(String::as_ref));
wxs_printer.product_icon(self.product_icon.as_ref().map(|s| s.as_str()));
wxs_printer.product_name(self.product_name.as_ref().map(String::as_ref));
wxs_printer.upgrade_guid(self.upgrade_guid.as_ref().map(String::as_ref));

wxs_printer.build().run()?;
}
Ok(())
Expand Down
Loading

0 comments on commit 7a8a599

Please sign in to comment.