-
Notifications
You must be signed in to change notification settings - Fork 27
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 licenses workspaces #165
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 for the PR and the fix. This does appear to be an oversight with the license generation.
Would it be possible to get a unit and/or integration test that would ensure the license builder works with the package
argument?
Yes of course! #[test]
#[serial]
fn workspace_package_works() { So I commented all the tests, except this one. test workspace_package_works ... FAILED
failures:
---- workspace_package_works stdout ----
thread 'workspace_package_works' panicked at 'assertion failed: result.is_ok()', tests/initialize.rs:833:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
After some debugging, it seems that the generated packages don't have an 'authors' field which is tested. Did I do something wrong or is this current test broken? |
@serpilliere You are on the right track and I would use the
You need WiX Toolset installed, which is a Windows only application, in order to pass some tests. You might run into some problems if you are trying to run the tests on a Linux machine.
You did nothing wrong. The test is broken. More specifically, the fixture is broken, and I have been slacking and not provided a fix yet. Please see #146 for some more information. You are probably using a version of Rust greater than v1.50. If you install an older version, the tests should run without error. |
Oh! I admin I didn't look at the issues linked to the tests. Sorry about that. Here is my understanding for the license regression test: let (eula_wxs_path, license_wxs_path) = match Eula::new(self.eula.as_ref(), &package)? { The eula result is an So in my understanding of the code, to make a regression test that triggers the bug, I need at least two conditions:
A third condition is also necessary on 'recent' cargo:
It also seems that cargo cannot generate a license field by default. So it seems we face the very same problem to generate a regression test: We need to modify the cargo toml file. In your existing tests code base, there are such a use case: toml.get_mut("package")
.map(|p| {
match p {
Value::Table(ref mut t) => {
t.insert(String::from("license"), Value::from("MIT"))
} So here is my proposition: The new regression test will use a code similar to the èworkspace_package_works` test, but will also customize the generated toml files Is it ok for you? |
@serpilliere Great work! This is all excellent. Your understanding and summary are correct and very useful.
This is okay with me and please proceed. A separate PR to fix the author field for newer versions of Cargo is going to be needed and we can use your insights as a template for that PR. Thank you. |
@serpilliere I have merged #166. Thanks for the contribution. With that merge, you should be able to add a function to modify the TOML file for the license as needed. |
d7fb453
to
9817132
Compare
I added the regression test. |
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.
Looks good.
Thank you for adding the test.
Thank you for the merge! |
The license is always generated using a
None
package with results in failing the initialization in workspaces environements.This is due to:
This PR adds the
package
field to the license creation code path in order to forward the desired package option.