Add native windows launcher to replace the current .bat files#24858
Add native windows launcher to replace the current .bat files#24858sbc100 wants to merge 1 commit intoemscripten-core:mainfrom
.bat files#24858Conversation
6c9f005 to
a6f3fc0
Compare
.bat files.bat files
|
I don't know much about windows, but would we need to build this as part of the emsdk, ship it, etc.? How important are the issues solved by this? |
I think since its just a tiny |
Isn't there windows-on-arm, so a single binary wouldn't work? |
True! Although I'm not sure anyone is using that yet. We certainly don't yet provide emsdk binaries for windows/arm64. |
|
I haven't reviewed the code yet but I think this idea is generally good We did something similar for the NaCl compiler (for a different reason, but a similar technique). I think I have a mild preference for building it with emsdk rather than checking it in, but I could be convinced. |
| } | ||
|
|
||
| int main() { | ||
| char exePath[MAX_PATH]; |
There was a problem hiding this comment.
Does this work with non-ascii directory/path names?
There was a problem hiding this comment.
There's a whole world of TCHAR pain and Win32 file API coming someone's way...
There was a problem hiding this comment.
The new versions handles all that I believe.
FWIW PowerShell scripts I added a while back should also already solve this issue, and they're chosen by default on Windows when users type I don't mind the |
Windows have long had system similar to macOS Rosetta, it's just that Windows on ARM has only picked up in popularity recently with new laptops, but essentially any x64 Native ARM might be nice to have for wasm-binaries, but for something as simple as this invocation script perf shouldn't matter as much. |
Having to maintain the Sadly I don't think the |
|
Perhaps worth looking at py2exe than rolling something own? |
I have looked at things like that over the years but the complexity always scared me off. It would also be little sad to loose the "edit-in-place" abilities of the plain-old-python files. |
|
I asked Gemini CLI to help me write a better version. Its seems to have done it! I asked it to find any discrpencies between these two and it came
The C code replaces the batch script by directly launching Python or ccache. It correctly determines the Python executable and the script to run. Key features include:
Potential Issues & Discrepancies:
Conclusion: The primary concern is the missing |
3aabac6 to
10988a1
Compare
.bat files.bat files
.bat files.bat files
59ff9e4 to
f10b565
Compare
|
Perf number seem fairly negligable: .exe: .bat: |
7ce0669 to
497c2bd
Compare
497c2bd to
d438601
Compare
|
I wonder if we should build the exes with the same compiler we use to build the emsdk binaries, just to minimize the chances of there being any interesting differences. I'm also not super enthusiastic about checking in binaries, but I guess if we want to maintain the invariant that you can just check out emscripten and use it without emsdk or other dependencies, then there aren't too many other options? |
d438601 to
866f3d3
Compare
Yes, perhaps we can do that on the emscripten-releases waterfall? I think that can happen later though, if we do run into issue.
Yes, I'm normally completely against that too, but I think this is good example of case where an exception that rule makes sense. Since the binary is so small we can think of it more like a binary shell script :) |
866f3d3 to
6af0046
Compare
dschuff
left a comment
There was a problem hiding this comment.
Since this is a fairly significant change, I wonder if there is a way we can make it "opt-in" first. Could we have both exe and bat files for a couple of releases and have people test?
I did keep the ability to opt out, but having them side-by-side complicates things I think. Are you suggesting something like:
Note: I don't think that this really works since users who just type I guess you don't like the current opt out mechanism of having to re-run the I guess one major downside of that mechanism is that the script does not exist in installed versions of emscripten (such as emsdk installs) because the |
6af0046 to
f664420
Compare
Yeah I guess so.
Speaking of which, which one would actually be selected? Seems like it ought to be deterministic.
It's not bad. Just wondering if it could be eaiser.
Yeah, a version that didn't require e.g. repackaging the emsdk install after running the script would be best. but yeah it seems like the opt-out should work for emsdk users if possible since that's most users. |
f664420 to
6aefc59
Compare
|
I think I'd like to land this now while we are early in the 5.0.3 cycle. If we run into any teething issues we will have time to either address them or improve the fallback so that normal emsdk users can use it too. I think trying to figure out how to include the fallback in the shipped version is more work that needs to be done at this point. |
|
Great to see work on this Sam. Though I do have to say I expect this to be a source of regression trouble.
My gut feeling here is that the .exe files will have the same long path limitation as the .bat files. I.e. the long path limitation on Windows is a limitation of the terminal, and not a limitation of running .bat files. Though I could be wrong.
Iiuc this issue was resolved already by the earlier PR? In any case, I have been working towards code signing the compiler executables at Unity, so (presuming I get that done before the next time we have to update) the Windows SmartScreen issue will likely not be a trouble for us at least, if I can manage to code sign the .exe wrappers for our distribution. LGTM overall. Good to see there is a fallback in interim. I was hoping a main benefit of this would be performance, as there would be some CPU overhead on Windows from having to launch the .bat files.. but reading the comment thread, that perf improvement apparently did not manifest? That is a bit disappointing. So I am not sure if I see a good benefit for this, however, happy to see this land, if nothing else, to learn more how well this kind of approach will work (and to understand if using .exe instead of .bat will indeed avoid having to use .rsp files) |
|
My own local test lab CI is a bit heavily on the red atm.. I'll try to see if I can get it to good state again, so it can test out this on Windows on ARM as well. |
Apparently .bat files do have a lower limit on command line length (8k chars I think) whereas windows has a much larger limit, but still limited compared to UNIX. This was pointed out to me in one of the comments above. So yes, I suppose it doesn't actually fully resolve that issue, but it will increase out limit to the same as other compilers on windows (e.g. gcc, clang, cl.exe). |
Also, just removing that workaround code is nice to have. |
|
Regarding the perf gains, I'm still now sure if we will see gains. I'll be curious to see, for example, if that full test suite speeds up on out waterfall (and your yours, @juj) |
This is still just an experiment but eventually I hope to remove the `.bat` files in favor of this executable. Users who have issues can opt out via `EMCC_USE_BAT_FILES`. There are several reasons to want to do this: 1. Batch files are notoriously painful to work with and mis-understood. 2. Should be faster (no need to launch cmd.exe). 3. Works around several known issues with .bat files including one that is known unsolvable one (see emcc.bat for more details)
6aefc59 to
13bb503
Compare
Its not clear if this will lead to issues with virus/security scanners. For this reason, I'm leaving the old
.batfilesaround for now and user can opt out of this by running
tools/maint/create_entry_points.py --bat-files.Assuming we don't run into any issues I hope to eventually completely remove support for the
.batfiles.There are several reasons to want to do this:
See: #26229
See: #19207