Skip to content

Call clang directly when cross-compiling from Windows to Android #572

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.

Sign up for GitHub

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

Merged
alexcrichton merged 11 commits into from
Dec 2, 2020
Merged

Call clang directly when cross-compiling from Windows to Android #572

Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
75e34d4
Generate response file when cross compiling from Windows to Android
maxded Nov 23, 2020
ad8fa55
Merge branch 'master' into android-cross-compile-response-files
maxded Nov 23, 2020
5cc37cb
Call clang directly when cross-compiling from Windows to Android
maxded Nov 25, 2020
45c7351
Remove debug println!s
maxded Nov 25, 2020
ebf1b45
Add -mstackrealign argument to some i686-linux-android targets
maxded Nov 26, 2020
6343b7c
Cargo fmt
maxded Nov 26, 2020
d8c2fb6
Address feedback
maxded Dec 1, 2020
58925ec
Address feedback
maxded Dec 1, 2020
ba59919
Address feedback
maxded Dec 2, 2020
8704a8e
Remove parse unwrap
maxded Dec 2, 2020
f4a1bce
Cargo fmt
maxded Dec 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2096,6 +2096,38 @@ impl Build {
tool
};

// New "standalone" C/C++ cross-compiler executables from recent Android NDK
// are just shell scripts that call main clang binary (from Android NDK) with
// proper `--target` argument.
//
// For example, armv7a-linux-androideabi16-clang passes
// `--target=armv7a-linux-androideabi16` to clang.
//
// As the shell script calls the main clang binary, the command line limit length
// on Windows is restricted to around 8k characters instead of around 32k characters.
// To remove this limit, we call the main clang binary directly and contruct the
// `--target=` ourselves.
if host.contains("windows") && android_clang_compiler_uses_target_arg_internally(&tool.path)
{
let mut f = || -> Option<bool> {
let file_name = tool.path.file_name()?.to_str()?.to_owned();
let (target, clang) = file_name.split_at(file_name.rfind("-")?);

tool.path.set_file_name(clang.trim_start_matches("-"));
tool.path.set_extension("exe");
tool.args.push(format!("--target={}", target).into());

for version in 16..=23 {
if target.contains(&format!("i686-linux-android{}", version)) {
tool.args.push("-mstackrealign".into());
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Nov 29, 2020

Choose a reason for hiding this comment

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

Is this what the current scripts do? Is there a reason to perhaps not pass this unconditionally? (I have no idea what this flag does)

Copy link
Copy Markdown
Contributor Author

@maxded maxded Dec 1, 2020

Choose a reason for hiding this comment

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

Yea that's what the scripts do. Only the i686-linux-android16-23 have the -mstackrealign option added in the script, which force realigns the stack at entry of every function. Don't think we want to do that unconditionally as I am not sure what the flag does as well.

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Dec 1, 2020

Choose a reason for hiding this comment

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

Hm ok, can you add a comment for why this arg is added? Also, is 23 the latest version? Or do newer versions not require this argument?

Copy link
Copy Markdown
Contributor Author

@maxded maxded Dec 1, 2020

Choose a reason for hiding this comment

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

Newer versions do not require the arguments

Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Dec 2, 2020

Choose a reason for hiding this comment

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

Hm ok, in that case could this perhaps be tweaked a bit differently to instead of looping over versions to instead take a look at the target and try to parse a version at the end? If this no longer happens then this is basically just handling older historical compilers then.

Copy link
Copy Markdown
Contributor Author

@maxded maxded Dec 2, 2020

Choose a reason for hiding this comment

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

Done!

}
}

Some(true)
};
f();
Copy link
Copy Markdown
Member

@alexcrichton alexcrichton Nov 29, 2020

Choose a reason for hiding this comment

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

I think the inner closure should be ok here to remove, it's fine to just have one if let for the file_name binding and avoid the closure business.

}

// If we found `cl.exe` in our environment, the tool we're returning is
// an MSVC-like tool, *and* no env vars were set then set env vars for
// the tool that we're returning.
Expand Down