Skip to content

Conversation

@SquallATF
Copy link

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

@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
Copy link
Author

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

@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.

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.

2 participants