Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

wngr
Copy link
Contributor

@wngr wngr commented Dec 4, 2020

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 wngr requested review from Dylan-DPC-zz and a team as code owners December 4, 2020 14:39
@wngr
Copy link
Contributor Author

wngr commented Feb 5, 2021

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

src/cli.rs Outdated
}

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I was confused.

src/cargo.rs Outdated
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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I'd appreciate another look now.

@UebelAndre
Copy link
Contributor

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

@wcampbell0x2a
Copy link

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

@TotalKrill
Copy link

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

Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a changelog entry.

src/cargo.rs Outdated Show resolved Hide resolved
@wngr 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
@Emilgardis
Copy link
Member

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]>
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());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Emilgardis mentioned this pull request Apr 6, 2022
@Emilgardis
Copy link
Member

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
Copy link
Contributor

otavio commented Jun 2, 2022

Cloud this be closed in favour of #684 ?

@Emilgardis
Copy link
Member

implemented/included in #684, thanks!

@Emilgardis Emilgardis closed this Jun 8, 2022
@datdenkikniet
Copy link

datdenkikniet commented Jun 8, 2022

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
Copy link
Member

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

@datdenkikniet
Copy link

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

@Emilgardis
Copy link
Member

No worries!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants