Silence echo in do command within run_python_compiler.bat

[MarcosASandoval]

This is a proposed solution to: Issue 25378

The emscan-deps.bat file produces invalid dynamic dependency files, suffix .ddi, due to the do command being echoed. The output of the scan step is used to create a temporary file that is then renamed into the built .ddi file.

Additionally, this merge has a small documentation improvement.

Example of emscripten build generated file:

D:\emscriptenDebug\out>() 
{
  "revision": 0,
  "rules": [
    {
      "primary-output": "CMakeFiles\\demoApp.dir\\demoApp.cpp.o"
    }
  ],
  "version": 1
}

This file should be a validly formed JSON document, instead we have a blank line and a line produced by the do command preceding the JSON document

This commit represents one proposed solution

Thank you for providing this awesome tool!

[juj]

Thanks for the patch, this looks good.

Would you be able to create a new unit test in test/test_other.py that surfaces this bug, and the fix in this PR would then fix?

The Windows .bat file stuff is so notoriously brittle due to Microsoft's refusal to maintain and develop it further, so every failure we get from there would definitely warrant to have a test case that will ensure that any refactors to those files will not be able to break any of the known uses.

[sbc100]

Can you also run this script so that emcc.bat and em++.bat are regenerated based on it?

[sbc100]

The emscan-deps.bat file is actually created from the run_python.bat file, not the run_python_compiler.bat file so can you update that too?

[sbc100]

If this command echos to stdout does that mean that this is getting echoed for ever emcc or em++ call?

If so, I wonder why more windows users have not noticed this? Perhaps the block in question is normally skipped? If so, can we write a test for this that causes the block not to be skipped?

[sbc100]

It looks like we do already have test that is supposed to catch this. See test_no_extra_output from #20916.

I guess we need to extend that test case so that the block in question here is not skipped. It looks like that maybe mean calling emcc or em++ in a certain specific way that makes "%~f0" not work. See microsoft/terminal#15212

[MarcosASandoval]

I ran the create_entry_points.py script to generate the rest of the files.

Working on creating a test to surface this bug.

Running the failing command manually, with small edits, produces a valid .ddi file

As to why Windows users seemingly haven't run into issues before, this do block is always run, it's not part of the conditional "%~f0" in these batch scripts.

This issue seems to come from running the scan deps build step with Ninja as it's the only way the erroneous lines are added to the output .ddi file

Command being run manually, TLDR: unwrapped the cmake -E rename ... command removing the quotes and shortening the cmake executable name:

C:\WINDOWS\system32\cmd.exe /C ""C:/Users/user/emsdk/upstream/emscripten/emscan-deps" -format=p1689 -- C:\Users\user\emsdk\upstream\emscripten\em++.bat   -std=gnu++23 -x c++ D:/emscriptenDebug/demoApp.cpp -c -o CMakeFiles\demoApp.dir\demoApp.cpp.o -MT CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi -MD -MF CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.d > CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.tmp" >  out\CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.tmp && cmake -E rename out\CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.tmp out\CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi

vs command run by Ninja:

C:\WINDOWS\system32\cmd.exe /C ""C:/Users/user/emsdk/upstream/emscripten/emscan-deps" -format=p1689 -- C:\Users\user\emsdk\upstream\emscripten\em++.bat   -std=gnu++23 -x c++ D:/emscriptenDebug/demoApp.cpp -c -o CMakeFiles\demoApp.dir\demoApp.cpp.o -MT CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi -MD -MF CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.d > CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.tmp && "C:/Program Files/CMake/bin/cmake.exe" -E rename CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi.tmp CMakeFiles\demoApp.dir\demoApp.cpp.o.ddi"

[MarcosASandoval]

Any thoughts on how to reproduce via test? Seems like we have to trigger the whole build in order to get Ninja to run.

[sbc100]

I'm looking into the test now.. hopefully I can upload this PR with the test failing and you can then fix it your PR: #25435

Or you can just use that as inspriation for a test here. Specifically I don't think you need be running scandeps at all to repro, should be doable with emcc itself.

[sbc100]

As to why Windows users seemingly haven't run into issues before, this do block is always run, it's not part of the conditional "%~f0" in these batch scripts.

Should that be "this do block is not always run"?

[MarcosASandoval]

As to why Windows users seemingly haven't run into issues before, this do block is always run, it's not part of the conditional "%~f0" in these batch scripts.

Should that be "this do block is not always run"?

Negative. The line @for %%I in (%~n0.bat) do ( iterates through all items in set (%~n0.bat) with each iteration setting %I to the currently iterated item. (%~n0.bat) is a one item set expanded to {scriptName}.bat, in this case emscan-deps.bat; thus always running.

Edit: Yes. This should read, "this do block is not always run"

[sbc100]

As to why Windows users seemingly haven't run into issues before, this do block is always run, it's not part of the conditional "%~f0" in these batch scripts.

Should that be "this do block is not always run"?

Negative. The line @for %%I in (%~n0.bat) do ( iterates through all items in set (%~n0.bat) with each iteration setting %I to the currently iterated item. (%~n0.bat) is a one item set expanded to {scriptName}.bat, in this case emscan-deps.bat; thus always running.

But isn't here a goto to lines up that skips the whole thing?

[sbc100]

I'm having trouble reproducing in the form of at test. Can you see what I might be missing here: #25435? Are you able to reproduce locally by running "/path/to/emcc" -c test.c?

[sbc100]

Ah.. I verified it with "emcc" -c even though it didn't happen with "/full/path/to/emcc" -v .. which is odd because your case it looks like you using the full path to emscan-deps.

In any case it looks like the test in #25435? can verify this fix.

[sbc100]

Ok, my change has landed so you should be able to simply modify test_shell_cmd_with_quotes and remove the if not WINDOWS check.

[MarcosASandoval]

@sbc100 Made the requested changes.

Thanks for the help getting me familiar with Emscripten's tests!

[sbc100]

Looks like there must be another missing @ since the test is still echoing the CWD (\nC:\\Users\\circleci\\AppData\\Local\\Te[53 chars]) \n')

[MarcosASandoval]

Locally, test_other.other.test_shell_cmd_with_quotes fails without the @ and passes with the @

I can't reproduce whatever is happening in the CircleCI build.

[MarcosASandoval]

This might be a silly question, but does it look like the .circleci/config.yml file defines the "Install emsdk" command as installing TOT (tip of tree)? Might mean these tests are run on the un-silenced do blocks

[sbc100]

This might be a silly question, but does it look like the .circleci/config.yml file defines the "Install emsdk" command as installing TOT (tip of tree)? Might mean these tests are run on the un-silenced do blocks

This just means that llvm and binaryen are coming from tot emsdk. The emscripten directory where all these bat files live is always coming from the git checkout of the PR branch.