feat: Parse Cargo's `--manifest-path` option to determine mounted docker root #494

wngr · 2020-12-04T14:39:10Z

This commits adds support for parsing the --manifest-path option to cross. So
far, the Cargo.toml manifest of the crate (or its Cargo workspace) to compile
has been assumed to be in the current working directory. This means, that
relative crate dependencies were not supported, because those paths were not
mapped into the docker container.

Take the following example structure, where my-bin depends on my-lib:

.
├── my-bin
│   ├── Cargo.toml
│   └── src
│       └── main.rs
└── my-lib
    ├── Cargo.toml
    └── src
        └── lib.rs

This commits enables such scenarios, by running cross from . like so:
cross build --manifest-path=my-lib/Cargo.toml --target x86_64-pc-windows-gnu,
as . is mapped as the container's root, and the options passed through to
Cargo.

Related #388 #139 #277 #78 #438

Co-authored-by: Kviring Alexey [email protected]

wngr · 2021-02-05T09:35:38Z

Would appreciate some feedback on the PR -- is something missing? Would you consider merging this feature upstream?

reitermarkus · 2021-02-20T18:39:16Z

src/cli.rs

 }

 pub fn parse(target_list: &TargetList) -> Args {
 let mut channel = None;
 let mut target = None;
+ let mut project_dir: Option<PathBuf> = None;


Can you make this be the manifest_path instead? That way we could use the same logic whether --manifest-path is specified or not. I. e. automatically find the Cargo.toml when running cross in a subdirectory as well.

Not sure I understand. Do you mean just renaming the variable?
project_dir is not necessarily manifest_path. That's the point of this PR to decouple the mounted root path from the location of the Cargo.toml file. So I'd say it's better to have a distinctive name for it.

project_dir is not necessarily manifest_path

From looking at the code, project_dir is current_dir/manifest_path, i.e. the absolute path to the Cargo.toml, isn't it?

Right, I was confused.

reitermarkus · 2021-02-27T07:07:28Z

src/cargo.rs

- let cd = env::current_dir().chain_err(|| "couldn't get current directory")?;
+pub fn root(manifest_path: Option<PathBuf>) -> Result<Option<Root>> {
+ let cd = match manifest_path {
+ Some(dir) => dir,


manifest_path is not a directory but is already a path to a Cargo.toml.

Thanks. I'd appreciate another look now.

UebelAndre · 2021-03-31T03:56:56Z

This would be a great change to be able to get in! 😃

wcampbell0x2a · 2022-01-12T04:17:30Z

Any updates on this? I would use this with actions-rs/cargo@v1, so a release of this would be awesome.

TotalKrill · 2022-02-14T13:53:50Z

We are currently using this rebased upon master with no issues building a huge project with microservices and interdependecies.

It has allowed us to migrate from copy pasting Cargo.toml workspace files depending on which software to build for which target to have the files include the correct libraries, to being able to build them much simpler. Please merge

reitermarkus

Please add a changelog entry.

src/cargo.rs

Emilgardis · 2022-03-17T18:23:58Z

Please rebase instead of adding merge commits, if you need guidance how just ask :D

root This commits adds support for parsing the `--manifest-path` option to cross. So far, the `Cargo.toml` manifest of the crate (or its Cargo workspace) to compile has been assumed to be in the current working directory. This means, that relative crate dependencies were not supported, because those paths were not mapped into the docker container. Take the following example structure, where `my-bin` depends on `my-lib`: . ├── my-bin │ ├── Cargo.toml │ └── src │ └── main.rs └── my-lib ├── Cargo.toml └── src └── lib.rs This commits enables such scenarios, by running cross from `.` like so: `cross build --manifest-path=my-lib/Cargo.toml --target x86_64-pc-windows-gnu`, as `.` is mapped as the container's root, and the options passed through to Cargo. Related cross-rs#388 cross-rs#139 cross-rs#277 cross-rs#78 Co-authored-by: Kviring Alexey <[email protected]>

Emilgardis · 2022-03-19T22:52:54Z

src/cargo.rs

+pub fn root(manifest_path: Option<PathBuf>) -> Result<Option<Root>> {
+ if let Some(manifest_path) = manifest_path {
+ if !manifest_path.is_file() {
+ eyre::bail!("No manifest found at \"{}\"", manifest_path.display());


we should be consistent about how we present paths

we currently have three ways of doing it

format!("{}", path) format!("`{}`", path) and format!(r#""{}""#, path)

don't know which one is best and it's not a blocker for this pr

Emilgardis · 2022-04-07T09:11:46Z

I think this is the wrong approach now, however it can be fixed without explicit support if we parse cargo metadata for the target (and other) folders. See #684

Am I wrong in this thinking?

otavio · 2022-06-02T17:46:03Z

Cloud this be closed in favour of #684 ?

Emilgardis · 2022-06-08T01:10:35Z

implemented/included in #684, thanks!

datdenkikniet · 2022-06-08T07:53:02Z

Could this PR be reopened @Emilgardis? The solution implemented in #684 does not solve the problem immediately described by/solved by this PR.

While the effect may be similar, it does not solve the problem if one is not using a workspace. The reason we can't/wish not to use a workspace is that compiling for multiple targets within one workspace can create conflicts with libc versions, as well as with other dependencies compiled against other system libraries.

Emilgardis · 2022-06-08T07:54:11Z

Have you tried the changes? It implements this pr, 09e5d0e in #684 is this pr.

datdenkikniet · 2022-06-08T08:03:45Z

Aho, apologies. Missed that bit :) Thanks for the response!

Emilgardis · 2022-06-08T08:05:48Z

No worries!

I'm curious about the libc clashes you mentioned, is it something like #724 maybe?

wngr requested review from Dylan-DPC-zz and a team as code owners December 4, 2020 14:39

reitermarkus reviewed Feb 20, 2021

View reviewed changes

reitermarkus reviewed Feb 27, 2021

View reviewed changes

wcampbell0x2a mentioned this pull request Jan 12, 2022

Reenable cross platform builds for CI rsadsb/adsb_deku#82

Closed

reitermarkus requested changes Mar 15, 2022

View reviewed changes

src/cargo.rs Outdated Show resolved Hide resolved

wngr requested a review from a team as a code owner March 17, 2022 18:07

wngr added a commit to wngr/cross that referenced this pull request Mar 17, 2022

mention cross-rs#494 in Changelog

2b496bc

wngr force-pushed the wngr/master branch from 2b496bc to a46b030 Compare March 19, 2022 19:02

Emilgardis approved these changes Mar 19, 2022

View reviewed changes

Emilgardis mentioned this pull request Apr 6, 2022

fix env #683

Merged

Emilgardis requested review from Emilgardis and removed request for Dylan-DPC-zz April 7, 2022 09:31

Emilgardis mentioned this pull request Apr 7, 2022

enable cargo workspace and path dependencies to work seamlessly #684

Merged

Emilgardis closed this Jun 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Parse Cargo's `--manifest-path` option to determine mounted docker root #494

feat: Parse Cargo's `--manifest-path` option to determine mounted docker root #494

wngr commented Dec 4, 2020 •

edited

Loading

wngr commented Feb 5, 2021

reitermarkus Feb 20, 2021

wngr Feb 22, 2021

reitermarkus Feb 22, 2021

wngr Feb 22, 2021

reitermarkus Feb 27, 2021

wngr Mar 26, 2021

UebelAndre commented Mar 31, 2021

wcampbell0x2a commented Jan 12, 2022

TotalKrill commented Feb 14, 2022

reitermarkus left a comment

Emilgardis commented Mar 17, 2022

Emilgardis Mar 19, 2022

Emilgardis commented Apr 7, 2022

otavio commented Jun 2, 2022

Emilgardis commented Jun 8, 2022

datdenkikniet commented Jun 8, 2022 •

edited

Loading

Emilgardis commented Jun 8, 2022

datdenkikniet commented Jun 8, 2022

Emilgardis commented Jun 8, 2022

feat: Parse Cargo's --manifest-path option to determine mounted docker root #494

feat: Parse Cargo's --manifest-path option to determine mounted docker root #494

Conversation

wngr commented Dec 4, 2020 • edited Loading

wngr commented Feb 5, 2021

reitermarkus Feb 20, 2021

Choose a reason for hiding this comment

wngr Feb 22, 2021

Choose a reason for hiding this comment

reitermarkus Feb 22, 2021

Choose a reason for hiding this comment

wngr Feb 22, 2021

Choose a reason for hiding this comment

reitermarkus Feb 27, 2021

Choose a reason for hiding this comment

wngr Mar 26, 2021

Choose a reason for hiding this comment

UebelAndre commented Mar 31, 2021

wcampbell0x2a commented Jan 12, 2022

TotalKrill commented Feb 14, 2022

reitermarkus left a comment

Choose a reason for hiding this comment

Emilgardis commented Mar 17, 2022

Emilgardis Mar 19, 2022

Choose a reason for hiding this comment

Emilgardis commented Apr 7, 2022

otavio commented Jun 2, 2022

Emilgardis commented Jun 8, 2022

datdenkikniet commented Jun 8, 2022 • edited Loading

Emilgardis commented Jun 8, 2022

datdenkikniet commented Jun 8, 2022

Emilgardis commented Jun 8, 2022

feat: Parse Cargo's `--manifest-path` option to determine mounted docker root #494

feat: Parse Cargo's `--manifest-path` option to determine mounted docker root #494

wngr commented Dec 4, 2020 •

edited

Loading

datdenkikniet commented Jun 8, 2022 •

edited

Loading