support compiler_param_file features on Windows

[LoSealL]

Highlights:

• When enable --feature=compiler_param_file on command line or .bazelrc file, _get_file can read the actual compile args in the file
• Fully compatible without this feature or change this feature during the developmemt.

Lowlights:

• When using compile commands extractor along with --feature=compiler_param_file, user must build the target once, because bazel aquery won't actually generate the .params file. (Which I think is a bazel limitation)
• Strip tail whitespaces and fix a typo whose charactor is not encoded as unicode..

[cpsauer]

For context for any other readers, this fixes #35. Worth reading that also.

[cpsauer]

Thanks so much for doing, @LoSealL!

I'm delighted that this route is working and really appreciate your good efforts here.
I'd love it if we cleaned things up just a bit before merging, though. Comments inline

[cpsauer]

Worth documenting the 8191--and why you chose a smaller limit? I assume because length calculate is an approximation of the real one with escaping https://docs.python.org/3/library/subprocess.html#converting-an-argument-sequence-to-a-string-on-windows

Another, (maybe better) approach could be try-except whatever exception it throws when the command is too long.

[LoSealL]

Sure, will add better doc here

[cpsauer]

S.g. Thoughts on the pro/con of the try-except approach?

[LoSealL]

try-except will be conflict with the following logic:

for line in header_search_process.stderr.splitlines():

We can't detect whether the error comes from compile failure or command line overflow. Not sure if errorcode can help.

[cpsauer]

Maybe I'm missing something, but I was thinking that there's probably a (Python) Error thrown if the line is longer than the command length limit (after escaping and all that). Like this one https://stackoverflow.com/questions/2381241/what-is-the-subprocess-popen-max-length-of-the-args-parameter? You could test by exceeding the limit to subprocess.run and seeing what it throws!
(Let's definitely not rely on parsing an error message out of stderr--your hardcoded 8000 is more reliable than that!)

[LoSealL]

@cpsauer Tested on my Windows 10 21H2:
For powershell, it can support user to type more than 8191 charactors on the console, while for cmd, user can't type any more if command line reaches 8191 charactors.
If you execute command from a script (i.e. .ps1 or .cmd/.bat), the cmd may ignore charactors that exceeds 8191 limitation, while powershell doesn't. For example:

CMD:

set a=1234...(more than 8191 chars)....EOL
:: will only show 1234....(to 8191 length), can't echo "EOL", and no error
echo %a%

PWSH:

$env:a="1234....(more than 8191 chars)...EOL"
# show full $a string, everything is OK
echo $env:a

But if your file path is too long (by default it can't longer than 254), most applications will fail with an error (just as your link) with both cmd and powershell console.

[cpsauer]

Ah, sure, but I think we're talking past each other somehow.

I meant that I'd guess Python would throw an Error if the args to subprocess.run were too long, even with check=False, rather then letting the error happen in the Windows shell. That is, is an error like the one linked thrown if you temporarily replace header_cmd in the subprocess command with "a "*10000 (or similar)?

[LoSealL]

Oh, got your point.
Just test and subprocess.call will return error code 1, with an stderr output "command line too long".

We may check return code 1 or stderr text (the text is in locale language)

[LoSealL]

Refer to https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-erref/1bc92ddf-b79e-413c-bbaa-99a5281a6c90
msvc compiler (only tested cl.exe) normally returns error code 2 (such as command error, compile error...)
We will use error code 1 for command line too long problem

[cpsauer]

Oh, bummer. So no Python error, then. Sorry for pushing in the wrong direction.

Do whatever you think is safest and most robust here. If you're confident code 1 is always this command-length-issue, then do that. And if not, then let's fall back to your old solution. (Again, sorry.)

[cpsauer]

Thanks so much, @LoSealL!

[cpsauer]

Thanks for documenting things so well, and sharing code via the local fn!

A few last things:

1. I didn't read the docs carefully enough on NamedTemporaryFile the first time. I'm now seeing

Whether the name can be used to open the file a second time, while the named temporary file is still open, varies across platforms (it can be so used on Unix; it cannot on Windows NT or later).

...which seems like it would cause problems. How do you think we should solve this? (Perhaps we do need to use tempfile.mkstemp after all...sorry.)
2) I think we want to be careful to make sure that we're flushing the write offer before calling subprocess run (or disabling buffering).

[LoSealL]

The doc in the source code says that NamedTemporaryFile does use mkstemp to create file... I'd like to double check that

[LoSealL]

def NamedTemporaryFile(mode='w+b', buffering=-1, encoding=None,
                       newline=None, suffix=None, prefix=None,
                       dir=None, delete=True, *, errors=None):
    """Create and return a temporary file.
    Arguments:
    'prefix', 'suffix', 'dir' -- as for mkstemp.
    'mode' -- the mode argument to io.open (default "w+b").
    'buffering' -- the buffer size argument to io.open (default -1).
    'encoding' -- the encoding argument to io.open (default None)
    'newline' -- the newline argument to io.open (default None)
    'delete' -- whether the file is deleted on close (default True).
    'errors' -- the errors argument to io.open (default None)
    The file is created as mkstemp() would do it.

    Returns an object with a file-like interface; the name of the file
    is accessible as its 'name' attribute.  The file will be automatically
    deleted when it is closed unless the 'delete' argument is set to False.
    """

    prefix, suffix, dir, output_type = _sanitize_params(prefix, suffix, dir)

    flags = _bin_openflags

    # Setting O_TEMPORARY in the flags causes the OS to delete
    # the file when it is closed.  This is only supported by Windows.
    if _os.name == 'nt' and delete:
        flags |= _os.O_TEMPORARY

    (fd, name) = _mkstemp_inner(dir, prefix, suffix, flags, output_type)
    try:
        file = _io.open(fd, mode, buffering=buffering,
                        newline=newline, encoding=encoding, errors=errors)

        return _TemporaryFileWrapper(file, name, delete)
    except BaseException:
        _os.unlink(name)
        _os.close(fd)
        raise

def _mkstemp_inner(dir, pre, suf, flags, output_type):
    """Code common to mkstemp, TemporaryFile, and NamedTemporaryFile."""

    names = _get_candidate_names()
    if output_type is bytes:
        names = map(_os.fsencode, names)

    for seq in range(TMP_MAX):
        name = next(names)
        file = _os.path.join(dir, pre + name + suf)
        _sys.audit("tempfile.mkstemp", file)
        try:
            fd = _os.open(file, flags, 0o600)
        except FileExistsError:
            continue    # try again
        except PermissionError:
            # This exception is thrown when a directory with the chosen name
            # already exists on windows.
            if (_os.name == 'nt' and _os.path.isdir(dir) and
                _os.access(dir, _os.W_OK)):
                continue
            else:
                raise
        return (fd, _os.path.abspath(file))

    raise FileExistsError(_errno.EEXIST,
                          "No usable temporary file name found")

Pasted the code here. It calls _mkstemp_inner which is same as mkstemp, and it uses a _RandomNameSequence to generate temp file name. TMP_MAX is 10000 to guarantee there's no name conflict.

Now the test code:

import os
import tempfile
import unittest
from concurrent.futures.thread import ThreadPoolExecutor


def _create_temp(bin):
    with tempfile.NamedTemporaryFile('w') as f:
        bin.append(f.name)
        f.write('abc')


class TestNamedTemporaryFile(unittest.TestCase):
    def test_mt_create(self):
        TEMP_BIN = []
        Q = 1000000

        pool = ThreadPoolExecutor(os.cpu_count())
        [pool.submit(_create_temp, TEMP_BIN) for _ in range(Q)]
        pool.shutdown(wait=True)
        self.assertEqual(len(TEMP_BIN), Q)
        self.assertEqual(len(set(TEMP_BIN)), Q)


if __name__ == '__main__':
    unittest.main()

Which is passed here.

[cpsauer]

Huh, interesting!

I wonder what part prompted them to add that reopen warning to the docs.

[cpsauer]

Aha! Looks like the thing that causes the problem with NamedTemporaryFile vs mkstemp in the source above is O_TEMPORARY.

Explained over at https://stackoverflow.com/questions/15169101/how-to-create-a-temporary-file-that-can-be-read-by-a-subprocess

And flushing comes up there, too.

---

I also want to make sure we aren't accidentally waiting on each other. I'm still waiting on you fixing this one before merging, right? And you're not waiting on me?

[LoSealL]

@cpsauer Ah, I got what you mean here, and there's indeed a potentail risk for cl.exe to read an opened temp file (while it works for me though...). Let's delete it manually as this post did.

[cpsauer]

I think index will already throw a ValueError if not found.

[LoSealL]

It just throws a KeyError, I think this exception will better let user know witch compile_args is missing.

[cpsauer]

[At least on my system]

>>>"".index("foo")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
ValueError: substring not found

But regardless, this is going to lead to a hard crash, right? Like this isn't something we expect to show the user.

[LoSealL]

But w/o this two lines it still goes to a hard crash if both /c or -c not found...

[cpsauer]

Totally agree!
It'll always say "-c not found...", though, bc of the previous line, though, right?
Not super opposed or anything, I'm not sure how much this adds.

[LoSealL]

You're right! What about change typo to

if compile_only_flag not in compile_args:
  raise ValueError(f"/c or -c (required for parsing sources) is not found in compile_args: {compile_args}")

[cpsauer]

Sure, but I really think this is okay without! They should always be present from Bazel, and we'd get a crash that user will report

[LoSealL]

OK, I will just delete them. Maybe I only encounter during my development.

[cpsauer]

Oh! I'm silly. You're adding it so that the contents of the array itself will be in the error message for easy debugging, right? That's nice, because then people will actually report the compile command as part of the error, even if we don't anticipate this case arriving during normal use.

(Sorry I was slow to understand.)

[cpsauer]

^ To be clear, if easy, could you re-add your code from #49 (comment). Again, my apologies for being slow to understand what you were up to.

[LoSealL]

@cpsauer I updated the commit

[cpsauer]

Sweet. But perhaps we should take one of the more robust answers from that stack overflow post?
[Also, any chance we could add a more direct link documenting the error code? I saw the 500 page pdf, but couldn't quickly find it therein.]

[cpsauer]

You may have a deeper understanding of what's going on in windows and O_TEMPORARY? Perhaps it's opened safely by msvc anyway? Not sure if you saw the part of that post about being able to reopen it using O_TEMPORARY? Still do think we should make sure we flush, though.

[LoSealL]

For O_TEMPORARY flag, you can refer to this page.
Briefly, trying to open a file created with flag O_TEMPORARY and not close yet, you must open with O_TEMPORARY as well, or you don't have the permission (PermissionError).
On the other hand, open a normal file is OK for both read and write, because the default flag is O_RDWR.

[LoSealL]

What do you mean by referring more robust answer? Can you paste the code?

And for CL error code, as this page said, it is the error code of Windows and there are lots of possible values...

[cpsauer]

Hey @LoSealL: I finally got back to my Windows machine and took a shot at the remaining things.

I think we definitely are all set on turning off compiler_param_file to aquery. ☑️ Simple, done, and clearly right.

For spilling arguments to command files when things get too long:
At least on my machine, I was seeing subprocess.run instead throwing winerror 206 when the command got too long, rather than returning error code 1--just as in that stack overflow link. So I redid things around catching winerror 206, and made the cleanup a bit more exception tolerant, etc. Interestingly, I was definitely seeing errors only for commands well over 8k. More like 30k. That would square with a call to CreateProcess instead of cmd.exe. (I was testing by adding, e.g.,['/EP'] * 10000 to header_cmd and seeing things run cleanly.)

That said, I'm still far from confident that we're handling all the cases right.
For example:

• Do we need to be quoting the arguments in the param file? (Bazel seems to do this, but always uses gcc format(!))
• Should they maybe be all on one line to avoid splitting arguments that have connected values? (Bazel seems to separate with newlines.)
• If your logic was working on your machine, why did I see the winerror at a higher limit and need the try-except on mine? (If this is thorny, one resolution would be to always spill over to a param file.)
• Is my .encode() [to utf-8, I believe by default] going to cause problems with your different code page? I assume so, so we'd better switch to os.fdopen
• etc.

Thoughts? Or ideas on how to do this well? I wonder if, e.g., SCons would have some good code to look at here.
I'd really like for us to get more confidence on it before we merge that part.
Very worse case we could punt and just not find headers if the command is too long, but that feels a little sad.

---

Also, separate, but any chance I could ask you to give the new exclude_headers = "all", exclude_headers = "external", exclude_external_sources = True a test on Windows, just to make sure they're working well there?

And more generally, if there are other Windows issues you're seeing, I'd love to know.

Thanks so much,
Chris

[LoSealL]

Hi @cpsauer . You are right.

Following your finds, I create a sample repo here. Which shows different behaviour depends on executables.
And for CL.exe, I think your solution is OK.

You can check that repo first to see if we still have diverges.

[LoSealL]

@cpsauer Updated:

I even can't reproduce my issue today, which I thought could caused by compile and link too many source files. Maybe it's solved by upgrading VS version?

So for now, we can make a conclusion here:

1. For compiler flags too long:
   * users need --features=compiler_param_file, and without it, bazel will report "command is longer than CreateProcessW's limit (32767 characters)" . (The 32767 is the limitation of CreateProcess API)
   * If user call cl.exe directly from command line, it worked somehow.
   * If user call cl.exe from python subprocess module, it will throw an exception with Windows error code 206. (This limitation is around 8151)

2. For linker flags too long or too many files to be compiled:
   * Both should work normally in bazel build. No need any features.

[LoSealL]

• Do we need to be quoting the arguments in the param file? (Bazel seems to do this, but always uses gcc format(!))

Add more tests to sample repo, please check there.

Bazel itself can't support path with space with compiler_param_file feature, so my conclusion is no need. If you find more escape cases, please add there to help us to check.

• Should they maybe be all on one line to avoid splitting arguments that have connected values? (Bazel seems to separate with newlines.)

Currently no issues found here.

• If your logic was working on your machine, why did I see the winerror at a higher limit and need the try-except on mine? (If this is thorny, one resolution would be to always spill over to a param file.)

Sorry, I can't reproduce either today. So it's right to stick to your solution.

• Is my .encode() [to utf-8, I believe by default] going to cause problems with your different code page? I assume so, so we'd better switch to os.fdopen

For temp file that write and read internally, utf-8 is OK, because we both write and read with utf-8. (CL.exe reads utf-8 normally).

Issues only happen when python reads a non-utf file. (i.e. a native notepad saved file, or echo command flush to a file with non utf-8 codepage.)

• etc.

[cpsauer]

Great! Thanks for working to clarify things, @LoSealL.

I quickly added some escaping in case Bazel ever fixes its implementation. And then I merged this in.

Could I ask you to proofread the latest version, test it, and report back?
I'd really appreciate it if you'd trigger the param file generation to make sure it all works. And could you also check out the "exclude" features above? (I'm back away from my Windows dev machine, unfortunately.)

[LoSealL]

Sure, also my many thanks to your great support!

[cpsauer]

And thank you for sticking with it! I know it's been a long adventure, and I really appreciate your help, start to finish

[cpsauer]

Some last miscellaneous things:

bazel will report "command is longer than CreateProcessW's limit (32767 characters)"

Just to confirm: You can aquery successfully without the compiler_param_file feature, right? And this only happens when running build actions?

And in your test repo, I'm wondering if the following explains what was going on:
echo -> Special because it's a builtin, so it envokes cmd.exe -> 8k command length limit and special message
For the others: 32k limit. Only cl.exe was over the limit because the parameters were shorter for the others?

[cpsauer]

Anyway, please do tell me if you have any concerns with what we've got here! I'd love to hear them.

And mostly, thank you for all your efforts and for checking and testing!

[cpsauer]

@LoSealL, could I ask one more Windows favor?
Could you run a bazel build on whatever repo you're using and see if there are files listing headers and other dependencies next to the .obj files? (under bazel-out) And if so, could you send me one, along with its path?

[To explain the importance: I'm trying to speed up the header finding by examining dependency files Bazel has cached. On macOS/Linux, there are .d files, listing headers in makefile format, and I'd like to know if there's something similar on Windows. fe5e048 is the commit if you're curious.]

[LoSealL]

@cpsauer No, there's no such .d like file on Windows. Instead, a .param file if --features=compiler_param_file, and the param file is a compiler command flags.