Use cargo-metadata instead of parsing TOMLs by hand #107

roblabla · 2020-09-30T15:04:31Z

Preface

This is a bit of a big PR, and the commits are currently a mess. I'm sorry.

Motivation

Currently, cargo wix manually parses TOML files, and tries to guess what the compiler is going to do based on this. This leads to a lot of issues: It doesn't properly support custom target directories, it fails to work in workspaces, it doesn't support cross-compilation (for instance to build 32-bit targets from a 64-bit host), it fails to find binaries under src/bin/*.rs. I'm sure there are more issues, but those are the ones I identified while trying to use cargo-wix in my own project.

Most of those issues could be fixed by asking cargo for the information we seek directly, instead of trying to guess them. This is exactly what the cargo metadata subcommand does: it allows us to query cargo for the metadata of a given project, for instance what packages are in the current workspace, which binaries that package contains, where those binaries will be built, etc...

Details

This PR does a few things:

First of all, it moves as much of the manual TOML parsing as possible to a call to cargo-metadata. If the current workspace has only one package, it will use this one, otherwise it requires the user to pass the package name via a command line parameter (--package).

Second, it uses the information from cargo-metadata to find all the binaries, including those in src/bin. Cargo is nice enough to give us all the implicit and explicit bin targets!

Third it removes all the hardcoded paths to target/. cargo-wix now injects a wix preprocessor variable, CargoTargetDir, that is set to the proper target directory (as found by cargo-metadata). The templates are updated to automatically use this variable.

TODO

Find a way to get the homepage. Cargo metadata doesn't provide it - we might need to parse the TOML to get that value :(.
EDIT: I submitted a cargo PR to surface those values: Homepage doc cargo metadata rust-lang/cargo#8744. If/when this is merged, this will need to be followed by a PR to the cargo_metadata project. This will likely take a bit. In the meantime, I guess we could manually parse the TOML just to get this field.
Add a --package arg to all the subcommands
Add tests for --package
Add tests for multi-bin projects

cargo wix now passes a variable called CargoTargetDir, with a path to the cargo target directory. This allows user to use a custom target dir, e.g. by setting the CARGO_TARGET_DIR env variable or setting target-dir in their cargo config.

roblabla · 2020-09-30T15:07:22Z

src/clean.rs

-        }
-
-        #[test]
-        fn target_wix_with_existing_file_but_not_cargo_toml_fails() {


This test was removed because it was testing for an error scenario that's... kinda wrong. It is 100% allowed to have a Cargo.toml whose name isn't Cargo.toml, if you pass it with --manifest-path.

I should probably re-add it, but explicitly check that it actually works instead of erroring.

roblabla · 2020-09-30T15:09:00Z

src/create.rs

-                        .join(TARGET_FOLDER_NAME)
-                        .join(WIX)
-                        .join(filename)
-                })
        } else {
            trace!("Using the current working directory (CWD) to build the WiX object files destination");


TODO: Change the trace to say it's relative to the cargo target dir

roblabla · 2020-09-30T15:10:43Z

src/create.rs

@@ -929,7 +875,7 @@ impl Execution {
        // for PathBuf, but it was unexpected and kind of annoying because I am
        // not sure how to add a trailing slash in a cross-platform way with
        // PathBuf, not that cargo-wix needs to be cross-platform.
-        dst.push(format!("{}\\", WIX));
+        let dst = target_directory.join("");


So, I had the same problem to add a trailing backslash... I essentially found two ways, this one, or

let s = target_directory.as_os_string(); s.push("\\"); PathBuf::from(s)

volks73 · 2020-10-01T21:30:18Z

Thank you for the excellent and detailed PR.

I have never been fond of the manual TOML parsing of the manifest, the guess work that was being used for project/package artifacts, and the hard-coded paths. I always thought there should be a better way to obtain package/project information from within a cargo subcommand. Side note, the "better way" is apparently a giant stream of (de)serialization: TOML->Cargo->JSON->cargo-metadata->cargo-wix. This feels a little cumbersome to me, but very much inline with command line interfaces. Despite this feeling, it is a much more robust and future-proof implementation; thus, I welcome and appreciate the relatively big PR.

You beat me to submitting an issue and PR to the Cargo project for the missing homepage field from the JSON output of the cargo metadata command. I agree, it will be a while before the change is propagated up to this project. Manual TOML parsing will still be needed for the homepage, unless you would prefer to wait until the change is available to eventually merge this PR?

Would it be possible for you to create an integration for a workspace project? Maybe this is what you mean by the "...multiple-bin" TODO?

I should be able to do a more in-depth review this weekend (Oct 3rd - Oct 4th). Did you want all of the TODOs completed before merging?

Thanks again and please keep the changes coming!

roblabla · 2020-10-01T22:02:34Z

Haha, thanks a lot. And thanks for the great project :).

Manual TOML parsing will still be needed for the homepage, unless you would prefer to wait until the change is available to eventually merge this PR?

Honestly, even if cargo merges the pr today it will take ~6 weeks to end up in stable, so it'd be a breaking change. Parsing the manifest manually in the meantime isn't very complex, and cargo gives us the full path to the manifest so it should be robust enough. So I think we should just do that.

Would it be possible for you to create an integration for a workspace project? Maybe this is what you mean by the "...multiple-bin" TODO?

So what I had in mind for multiple-bin is a single package containing multiple implicit binary targets in src/bin/*.rs.

I do have plans for full workspace support, but I think it should be a follow-up PR because it will need a bit of design work which is still todo. I'll explain some of the problems I foresee in a full workspace support tomorrow.

I should be able to do a more in-depth review this weekend (Oct 3rd - Oct 4th). Did you want all of the TODOs completed before merging?

Yeah, hopefully I should have everything done before merging. The remaining bits are rather simple.

volks73 · 2020-10-02T15:06:26Z

Would it be possible for you to create an integration for a workspace project? Maybe this is what you mean by the "...multiple-bin" TODO?

So what I had in mind for multiple-bin is a single package containing multiple implicit binary targets in src/bin/*.rs.

Cool, I understand and appreciate the inclusion of the test.

I do have plans for full workspace support, but I think it should be a follow-up PR because it will need a bit of design work which >is still todo. I'll explain some of the problems I foresee in a full workspace support tomorrow.

Yes! Implementing support for workspaces has been lagging but also I have never had a clear idea of how to implement it. There appears to be a lot of flexibility in organizing projcets with workspaces, so I would love to hear some ideas and approaches to addressing workspace support, but it makes sense to focus on that after this PR. I do not want to distract you.

Yeah, hopefully I should have everything done before merging. The remaining bits are rather simple.

You clearly are in the "flow" and making great progress on this PR. I will hold off and let you do your thing.

When passing a package to initialize, we should create the wix directory in the subpackage folder.

roblabla · 2020-10-02T16:54:58Z

src/initialize.rs

-        }
-
-        #[test]
-        fn destination_is_correct_with_input() {


All those tests are irrelevant now, since destination is always relative to the provided package.

roblabla · 2020-10-02T16:56:49Z

So, I think this is ready for review, I fixed all the small bugs that I could find, but there might be more hiding :^).

roblabla added 9 commits September 30, 2020 12:42

Migrate to cargo-metadata

58e70e3

Comment out test that won't work until homepage is fixed

0c3c44e

fixup! Migrate to cargo-metadata

81ced64

Override cargo config target dir in tests.

8871060

Get rid of every use of TARGET_FOLDER_NAME

1352ac2

Don't hardcode path to target dir in WiX XML

215000d

cargo wix now passes a variable called CargoTargetDir, with a path to the cargo target directory. This allows user to use a custom target dir, e.g. by setting the CARGO_TARGET_DIR env variable or setting target-dir in their cargo config.

Fix passing Cargo.toml path

c8bb53c

Fix custom_target_dir_works test

7eb48a3

Add target_directory to logs

b76e5d3

roblabla commented Sep 30, 2020

View reviewed changes

Simplify target dir setting logic

4f107d7

roblabla force-pushed the cargo-metadata branch 3 times, most recently from b1752e3 to 3c4e1af Compare September 30, 2020 15:28

Hopefully last fix for custom_target_dir

a9d4d19

roblabla force-pushed the cargo-metadata branch from 3c4e1af to a9d4d19 Compare September 30, 2020 15:30

volks73 assigned roblabla Sep 30, 2020

roblabla added 4 commits October 1, 2020 08:06

Fix the last remaining tests

3ca58aa

Fix clippy warnings

13a7751

Fix format errors

bf9591c

Fix input_works_outside_cwd test

b220734

roblabla force-pushed the cargo-metadata branch from 2159904 to b220734 Compare October 1, 2020 08:34

roblabla added 6 commits October 1, 2020 08:38

fixup! Fix format errors

d79b1a8

fixup! Fix input_works_outside_cwd test

05de555

Fix initialize::multiple_binaries_works test

c3fa601

fixup! Fix input_works_outside_cwd test

a5d27d9

Make package a flag instead of a positional arg

575fab2

Find wix sources in the right directory when using --package

bef60bd

volks73 added the enhancement label Oct 1, 2020

Re-enable homepage acquisition by parsing the manifest

e1d1e37

roblabla force-pushed the cargo-metadata branch from fe48723 to e1d1e37 Compare October 2, 2020 11:47

Add package flag to all relevant subcommands

e670ddd

roblabla force-pushed the cargo-metadata branch from c590bd1 to e670ddd Compare October 2, 2020 14:02

Initialize: Create wix directory in correct subdir

61f15a1

When passing a package to initialize, we should create the wix directory in the subpackage folder.

roblabla force-pushed the cargo-metadata branch from 37fb561 to 61f15a1 Compare October 2, 2020 15:29

roblabla added 5 commits October 2, 2020 16:02

Fix wxs_printer step of initialize using wrong package

1b8c90d

Add workspace tests

6c4e594

Avoid crashing when being given a wrong package name

b8bad99

fixup! Add workspace tests

d5f4c3a

Fix format

9eb3417

roblabla force-pushed the cargo-metadata branch from 25cd005 to 9eb3417 Compare October 2, 2020 16:17

roblabla added 4 commits October 2, 2020 16:18

'Fix' clippy warning

65a2838

Init logging in initialize workspace tests

2abc466

Fix misleading trace

8b4db91

Use the WIX variable instead of hardcoding the str

473c1e0

roblabla commented Oct 2, 2020

View reviewed changes

volks73 merged commit 8bdf500 into volks73:master Oct 3, 2020

volks73 mentioned this pull request Oct 4, 2020

Remove manual parsing of Cargo.toml for homepage and documentation #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cargo-metadata instead of parsing TOMLs by hand #107

Use cargo-metadata instead of parsing TOMLs by hand #107

roblabla commented Sep 30, 2020 •

edited

Loading

roblabla Sep 30, 2020 •

edited

Loading

roblabla Sep 30, 2020

roblabla Sep 30, 2020

volks73 commented Oct 1, 2020

roblabla commented Oct 1, 2020

volks73 commented Oct 2, 2020 •

edited

Loading

roblabla Oct 2, 2020

roblabla commented Oct 2, 2020

Use cargo-metadata instead of parsing TOMLs by hand #107

Use cargo-metadata instead of parsing TOMLs by hand #107

Conversation

roblabla commented Sep 30, 2020 • edited Loading

Preface

Motivation

Details

TODO

roblabla Sep 30, 2020 • edited Loading

Choose a reason for hiding this comment

roblabla Sep 30, 2020

Choose a reason for hiding this comment

roblabla Sep 30, 2020

Choose a reason for hiding this comment

volks73 commented Oct 1, 2020

roblabla commented Oct 1, 2020

volks73 commented Oct 2, 2020 • edited Loading

roblabla Oct 2, 2020

Choose a reason for hiding this comment

roblabla commented Oct 2, 2020

roblabla commented Sep 30, 2020 •

edited

Loading

roblabla Sep 30, 2020 •

edited

Loading

volks73 commented Oct 2, 2020 •

edited

Loading