xbuild/cargo: Linearize workspace detection #83

MarijnS95 · 2022-12-01T11:38:40Z

Import cargo workspace changes from cargo-subcommand in preparation for workspace inheritance.

This effectively synchronizes us with:

rust-mobile/cargo-subcommand#23
rust-mobile/cargo-subcommand#24
rust-mobile/cargo-subcommand#25
(and a tiny initial bit of rust-mobile/cargo-subcommand#12)

@dvc94ch the changes from rust-mobile/cargo-subcommand#25 are still WIP, would you mind checking them out with me (see TODOs) and deciding where to go next?

xbuild/src/cargo/manifest.rs

MarijnS95 · 2022-12-01T11:39:55Z

xbuild/src/cargo/mod.rs

+                // If a workspace was found, and the user chose a package with `-p`, find packages relative to it
+                // TODO: What if we call `cargo apk run` in the workspace root, and detect a workspace? It should
+                // then use the `[package]` defined in the workspace (will be found below, though, but currently
+                // fails with UnexpectedWorkspace)
+                utils::find_package_manifest_in_workspace(


Most notably this TODO: rust-mobile/cargo-subcommand#25 (comment)

@dvc94ch before re-approving: this is the only thing that really needs to be looked into....

@dvc94ch I ended up implementing this (and some more fixes) in rust-mobile/cargo-subcommand#25, can you please take a look at the last 3 commits? If you are okay with that approach I'll transplant the changes to xbuild!

xbuild/src/cargo/mod.rs

xbuild/src/cargo/utils.rs

xbuild/src/cargo/mod.rs

dvc94ch

looks good to me, thanks!

xbuild/src/cargo/utils.rs

xbuild/src/cargo/mod.rs

dvc94ch · 2022-12-01T12:25:34Z

I vaguely recall that at one point we agreed xbuild would use cargo-subcommand or a variation of it, but then the work was never done. what are your thoughts?

MarijnS95 · 2022-12-01T13:23:07Z

I vaguely recall that at one point we agreed xbuild would use cargo-subcommand or a variation of it, but then the work was never done. what are your thoughts?

Must be long enough ago that I don't remember, but since I have been maintaining cargo-subcommand and added useful features to it which xbuild lacks, I'm in favour of that (to reduce dual maintenance burden for now). Then we cannot do things like #84 though.

Do you want to pick that up?

dvc94ch · 2022-12-01T13:29:19Z

It's not really a priority for me right now as the minimum subset of cargo that xbuild implements works just well enough for my purposes. But I'm open to a PR.

MarijnS95 · 2022-12-01T13:59:06Z

Let's just keep on copy-pasting cargo-subcommand for now then; that only makes converting easier when we make the jump.

dvc94ch · 2022-12-03T11:37:45Z

So merging this?

MarijnS95 · 2022-12-03T16:05:39Z

@dvc94ch no, I first want to fix #83 (comment) both here and in cargo-subcommand. Should be simple but I've yet to come up with an elegant way code-wise. Will be a couple days until I can dedicate time to this though (and subsequently implement workspace inheritance).

When calling `.parent()` on a filename the result is `""`, which should be treated as PWD (`"."`) but `dunce::canonicalize()` ends up in a "No such file or directory".

`.ancestors()` only calls `.parent()` on a `Path` which walks up until the string empty, but this isn't the root of the filesystem if the path wasn't absolute.

Instead of relying on non-workspace logic.

dvc94ch

is fine. but distracts from what this crate is actually about, passing the correct flags and env variables to cargo build. Maybe all this logic can live in a root level crate?

MarijnS95 · 2022-12-06T17:23:15Z

but distracts from what this crate is actually about, passing the correct flags and env variables to cargo build.

xbuild was already parsing the Cargo.toml manifest before this PR, if you don't want it to maybe we should remove it altogether? It may be used for more nontrivial things than appear on the surface, though. On the other hand, if we keep it, it should at least somewhat mimic what cargo does, correctly.

Note that we also lack [env] support in .cargo/config.toml.

Maybe all this logic can live in a root level crate?

Where would you like/suggest to move this to? xcommon? Or a lean-er variant of cargo-subcommand so that I didn't have to transplant (and now maintain) the same code in two spots?

dvc94ch · 2022-12-06T17:29:50Z

no, xcommon was intended for utilities used across various file formats, this should not live in xcommon. just add a new top level crate, xbuild-cargo or whatever. yes it would be a leaner version of cargo-subcommand.

dvc94ch · 2022-12-06T17:32:12Z

actually, ideally we'd get rid of xcommon, not sure if it's even needed

MarijnS95 · 2022-12-06T17:33:08Z

Fine by me, I haven't really seen the need for a split there. Having it more modularized would definitely benefit us and external parties.

dvc94ch · 2022-12-06T17:36:41Z

so the reason for xcommon is for providing these utilities used by the appbundle, apk and msix crates related to image processing, zip files and signing:

use xcommon::{Scaler, ScalerOptsBuilder, Signer, Zip, ZipFileOptions, ZipInfo};

dvc94ch · 2022-12-06T17:37:00Z

why the llvm stuff is in there I don't really know

MarijnS95 commented Dec 1, 2022

View reviewed changes

xbuild/src/cargo/mod.rs Outdated Show resolved Hide resolved

dvc94ch reviewed Dec 1, 2022

View reviewed changes

xbuild/src/cargo/utils.rs Show resolved Hide resolved

xbuild/src/cargo/mod.rs Outdated Show resolved Hide resolved

MarijnS95 force-pushed the workspace-manifest branch from fd76efb to 03edef6 Compare December 1, 2022 13:30

MarijnS95 force-pushed the workspace-manifest branch 2 times, most recently from 3e13660 to f3f2e0a Compare December 1, 2022 14:04

dvc94ch approved these changes Dec 1, 2022

View reviewed changes

MarijnS95 and others added 6 commits December 6, 2022 11:13

xbuild/cargo: Linearize workspace detection

7bdc34d

utils: Support path canonicalization for empty strings

837802d

When calling `.parent()` on a filename the result is `""`, which should be treated as PWD (`"."`) but `dunce::canonicalize()` ends up in a "No such file or directory".

Canonicalize --manifest-path so that a workspace is always found

96f0741

`.ancestors()` only calls `.parent()` on a `Path` which walks up until the string empty, but this isn't the root of the filesystem if the path wasn't absolute.

Use workspace members to select package by --manifest-path or $PWD

948a76f

Instead of relying on non-workspace logic.

Pre-parse all workspace manifests to check for errors

3618b01

Pre-parse all member manifests for ByName selection too

60e43e5

MarijnS95 force-pushed the workspace-manifest branch from f3f2e0a to 60e43e5 Compare December 6, 2022 12:23

MarijnS95 mentioned this pull request Dec 6, 2022

Inherit package configuration (version and description) from workspace root #90

Merged

MarijnS95 requested a review from dvc94ch December 6, 2022 16:34

dvc94ch reviewed Dec 6, 2022

View reviewed changes

MarijnS95 merged commit d5ff1af into master Dec 6, 2022

MarijnS95 deleted the workspace-manifest branch December 6, 2022 17:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xbuild/cargo: Linearize workspace detection #83

xbuild/cargo: Linearize workspace detection #83

MarijnS95 commented Dec 1, 2022

MarijnS95 Dec 1, 2022

MarijnS95 Dec 1, 2022

MarijnS95 Dec 6, 2022

dvc94ch left a comment

dvc94ch commented Dec 1, 2022

MarijnS95 commented Dec 1, 2022

dvc94ch commented Dec 1, 2022

MarijnS95 commented Dec 1, 2022 •

edited

Loading

dvc94ch commented Dec 3, 2022

MarijnS95 commented Dec 3, 2022

dvc94ch left a comment

MarijnS95 commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

MarijnS95 commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

xbuild/cargo: Linearize workspace detection #83

xbuild/cargo: Linearize workspace detection #83

Conversation

MarijnS95 commented Dec 1, 2022

MarijnS95 Dec 1, 2022

Choose a reason for hiding this comment

MarijnS95 Dec 1, 2022

Choose a reason for hiding this comment

MarijnS95 Dec 6, 2022

Choose a reason for hiding this comment

dvc94ch left a comment

Choose a reason for hiding this comment

dvc94ch commented Dec 1, 2022

MarijnS95 commented Dec 1, 2022

dvc94ch commented Dec 1, 2022

MarijnS95 commented Dec 1, 2022 • edited Loading

dvc94ch commented Dec 3, 2022

MarijnS95 commented Dec 3, 2022

dvc94ch left a comment

Choose a reason for hiding this comment

MarijnS95 commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

MarijnS95 commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

dvc94ch commented Dec 6, 2022

MarijnS95 commented Dec 1, 2022 •

edited

Loading