search

mozilla / rust-android-gradle

Apache License 2.0
1.08k stars 70 forks source link

Correct linker-wrapper script under Msys2 and Cygwin. Fixes #58 #59

Open SquallATF opened 3 years ago

SquallATF commented 3 years ago

Because rustc can not run shell script under windows bash, so make a hack in linker-wrapper.bat, if SEHLL variable is defind and find cygpath then try run shell script. Otherwise the default behavior will be used Another fix is convert RUST_ANDROID_GRADLE_CC to unix path when run with native python under Msys2 and Cygwin. if run with native python under Msys2 and Cygwin the subprocess need executable file path in unix format.

ncalexan commented 3 years ago

@SquallATF thanks for the patch. This is more complicated than I am comfortable, mostly because you choose to "do it right" and use ctypes to invoke Cygwin/MSYS path conversions, but I can live with that.

However, I'd like to understand why you bounce from .bat -> .sh -> python rather than go straight from .bat -> python. Is there a technical reason for that?

Finally, I have ticket #49 to stop using Python for the linker wrapper entirely. I don't think it's needed, since we're just invoking. If you were to instead fix #49, and just invoke in the .bat file (which I'm pretty sure I didn't know how to do when I wrote this thing), and then you were to handle the Cygwin/MSYS case, we would no longer need to do any ctypes stuff. Right? What do you think of that approach?

If there's some hard blocker to doing #49 and then this work, I'm inclined to accept this patch: I have only nits and style preferences. Thanks again!

SquallATF commented 3 years ago

Update patch, remove python path convert and convert it from linker-wrapper.bat

if use .bat -> python, will break link. when linker arguments too long, rustc will create a linker response file, but native Msys2/Cygwin application will convert response file back to argument when run in cmd, then will get argument too long error. so that need run python from .sh when use Msys2/Cygwin

ncalexan commented 3 years ago

@SquallATF I'm sorry that I haven't responded -- I've been very busy with work and family. I expect to merge this fix but I haven't had time to work through the "don't use Python" details myself just yet. Thanks again, for the patch and for your patience.