Skip to content

Support sub project build without parent Cargo.toml. #438

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

Closed
wants to merge 4 commits into from
Closed

Support sub project build without parent Cargo.toml. #438

wants to merge 4 commits into from

Conversation

avkviring
Copy link
Contributor

No description provided.

@avkviring avkviring requested review from Dylan-DPC-zz and a team as code owners June 29, 2020 17:05
@avkviring
Copy link
Contributor Author

avkviring commented Jun 29, 2020

Hi, I have some project.

- root
-- Client/Cargo.toml
-- Common/Cargo.toml
-- Server/Cargo.toml

This command don't work:
cross build --manifest-path clients/Cargo.toml --target armv7-linux-androideabi --release

This PR fixed this problem.

src/cli.rs Outdated
let mut target_dir = None;
let mut sc = None;
let mut all: Vec<String> = Vec::new();

{
let mut args = env::args().skip(1);
while let Some(arg) = args.next() {
if all.last().unwrap_or(&"".to_string()) == "--manifest-path" {
Copy link
Member

Choose a reason for hiding this comment

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

Why all.last() when we are using an iterator? Use the same pattern as for the other flags below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

src/cli.rs Outdated
}

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

Choose a reason for hiding this comment

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

Use a PathBuf to store this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

src/cli.rs Outdated
let mut target_dir = None;
let mut sc = None;
let mut all: Vec<String> = Vec::new();

{
let mut args = env::args().skip(1);
while let Some(arg) = args.next() {
if all.last().unwrap_or(&"".to_string()) == "--manifest-path" {
project_dir = Option::Some(format!("{}/{}", env::current_dir().expect("").to_str().unwrap(), arg.to_string()));
Copy link
Member

Choose a reason for hiding this comment

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

Use Path::join instead of format!.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

@reitermarkus
Copy link
Member

reitermarkus commented Jun 29, 2020

This command don't work:
cross build --manifest-path clients/Cargo.toml --target armv7-linux-androideabi --release

Can you post the output of the failed command?

src/cli.rs Outdated
if let ("+", ch) = arg.split_at(1) {
if arg == "--manifest-path" {
all.push(arg);
let path = args.next().expect("Missing argument in --manifest-path");
Copy link
Member

Choose a reason for hiding this comment

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

Use if let Some(..) for this and also add the case for --manifest-path=.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This command don't work:
cross build --manifest-path clients/Cargo.toml --target armv7-linux-androideabi --release

Can you post the output of the failed command?

It is fallback to host cargo and fail in linking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand :-( I am newbies in rust, please help me.

Copy link
Member

Choose a reason for hiding this comment

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

Look at the code below, there it is used in the same way.

Copy link
Member

Choose a reason for hiding this comment

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

if let Some(path) = args.next() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fix

@@ -26,7 +26,7 @@ impl Subcommand {
_ => true,
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Please fix the trailing whitespace everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix

Copy link
Member

Choose a reason for hiding this comment

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

The whitespace is still there, please remove it.

@avkviring avkviring requested a review from reitermarkus June 30, 2020 05:37
@@ -26,7 +26,7 @@ impl Subcommand {
_ => true,
}
}

Copy link
Member

Choose a reason for hiding this comment

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

The whitespace is still there, please remove it.

{
let mut args = env::args().skip(1);
while let Some(arg) = args.next() {
if let ("+", ch) = arg.split_at(1) {
if arg == "--manifest-path" {
Copy link
Member

Choose a reason for hiding this comment

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

Still needs the case for --manifest-path=.

@reitermarkus
Copy link
Member

@avkviring, any update on this?

@avkviring
Copy link
Contributor Author

@avkviring, any update on this?
Hi, I have vacation and don't have computer now :-(

@wngr
Copy link
Contributor

wngr commented Dec 2, 2020

I'd be interested in taking this over, if there is no disagreement.

Edit: I addressed the outstanding comments in https://github.com/wngr/cross/tree/wngr/master
However, there are a bunch of changes on the files from running cargo fmt -- is this projected manually formatted?

@avkviring
Copy link
Contributor Author

Hi, it is very good news :-)

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.

3 participants