search

mozilla / rust-android-gradle

Apache License 2.0
1.11k stars 71 forks source link

Invoke wrong linker-wrapper script under Msys2 #58

Open SquallATF opened 3 years ago

SquallATF commented 3 years ago

https://github.com/mozilla/rust-android-gradle/blob/aff8cdb1fbb9d28469cff98ab6c5ed60a764e12c/plugin/src/main/kotlin/com/nishtahir/CargoBuildTask.kt#L183

if run gradlew under Msys2 or Cygwin and use windows java will also report os.name is Windows, so that will always invoke linker-wrapper.bat not linker-wrapper.sh. but if Cygwin native command run under winshell, will expand linker response file to command arguments and that cause Argument list too long error. https://github.com/cygwin/cygwin/blob/44eb4163234089e8830b018dbcf6a08333eab202/winsup/cygwin/dcrt0.cc#L337-L343

Wish test if shell is bash then run linker-wrapper.sh not linker-wrapper.bat under windows.

ncalexan commented 3 years ago

@SquallATF this is certainly not an environment we've tried before. I'm surprised that the JVM under Cygwin is reporting os.name == "Windows", but I guess it does.

I'd certainly review a patch to make this condition handle your use case. What do you propose? Perhaps we could look at some environment variables (say, SHELL)? Actually OSTYPE looks hopeful. What do you think?

ncalexan commented 3 years ago

Sounds like OSTYPE is a bash-ism; maybe we check that and fall back to uname? I'm not sure what's best and most efficient here.

SquallATF commented 3 years ago

I think just test SHELL it is ok, OSTYPE is not a environment variables. image

another probleam is rust can not run linker-wrapper.sh directly. https://github.com/rust-lang/rust/blob/6fd7a6dc0f5c4c8fd51d57c0f4f795d52481f904/compiler/rustc_codegen_ssa/src/back/link.rs#L154-L171 image

ncalexan commented 3 years ago

Huh, I didn't realize that OSTYPE was a built-in and not an environment variable. How irritating.

What do you propose to do about Rust invoking the linker wrapper under Cygwin? I suppose that we can invoke $SHELL linker-wrapper.sh ..., if that works in practice? Or do we still want to use the .bat file under Cygwin?

Basically, I'll happily review a patch for this, but I'm not going to address it myself.