nit: / shouldn't be needed here.
the difference between %t and %/t is the latter is expressed with forward slashes on every platform.
nit: maybe create a new Inputs directory under test/remote-index and move the files there?
use %python instead to enable lit substitution, just in case there are weird build bots without python on the PATH or the default one pointing at a wrong version.
can we use 127.0.0.1:50051 instead? as starting a server on 0.0.0.0 might be blocked on some bots again. And also I would rather use a random port number (greater than 1024, possibly by letting the python script choose the number, and getting rid of the argument completely), to lower the chances of failing accidentally when 50051 is in use. (it might be because bot is running multiple tests in parallel, or bot itself has a grpc server running on default port for whatever reason).
can we rename test-directory to project-root and pass it directly from lit ?
this will also contain \\ when run on windows.
do we really need the abspath conversion here ? is it to get rid of .. ? i am uneasy about this as the call is likely to introduce \\.
can we instead wait until server prints Server listening on ... ?
can we just wait until clangd process shuts down ?
I think we should do this, because we also pass this address into --remote-index-address= clangd parameter, which is illegal. Seems Linux and Mac allow to call connect with 0.0.0.0 as a destination address, but Windows does not and this test fails.
Is there any reason to use %/S in --project-root? We support --project-root with native slashes (at least --project-root=%S works for me on Windows + MS compiler)
It looks more reasonable to use %/S in paths like this %/S/Inputs/Source to avoid mixing slashes on Windows (but it works correctly with mixing slashes anyway)
It works ok now.
we should rather call lit.llvm.llvm_config.use_default_substitutions(), preferably right after use_clang() on line 4.
sorry for the churn, i thought we were already doing this.
nit: maybe perform the magic after arg parsing ?
this still binds the socket to all interfaces, can we specify localhost or 127.0.0.1 explicitly for the interface address.
nit: formatting looks off, is this really what yapf offers?
i would rather scan through all lines and look for Server listening on $server_addres. We should be able to either hit the end of stream (i.e. server encounters a failure and shuts down) or find the log message we are looking for.
just to make test less reliant on log ordering.
can we rather use signal.SIGXXX here instead of 9 ?
Also rather than kill, SIGINT might be more applicable. https://docs.python.org/3/library/signal.html#signal.SIGKILL claims sigkill is not available on windows.
Resolve post-LGTM commentts, increase index hot reload frequency.
Yep, I'm formatting with it :(
Unfortunately, it's not as easy :( Then I would have to set a time to wait for the server to warm up and read lines then only. The problem here is that the server runs, doesn't shut down but also doesn't print the message. We'll hang the buildbots then.
But I'll go with the timeout option. I don't think it's better than checking the first line as the initialize one but still.
The problem with SIGINT is that it needs to wait for the hot reload sync to finish :) I'll add a patch to modify the frequency via CLI to avoid long test terminaiton.
Why? isn't the readline() on stderr blocking ? If not how was this code working without sleep before?
If the argument above is correct, this was the case already, i.e. stderr.readline() would block until a line is available or server had shutdown.
To overcome this problem I would suggest having a separate thread that would kill the server if it is still running after N seconds.
so if we think .readline() is blocking but this is not, can't we just keep calling readline instead?
i was checking llvm's lit.site.cfg.py.in for reference and it was using python3 too.
any specific reasons for using PYTHON_EXECUTABLE instead of python3? I can't say this was a strong preference, but the python script included might not be python2 compliant, hence the need for 3.
If there is a compelling reason for using a possibly python2 executable, we can try to make the script both 2 and 3 compliant.
Oh interesting, I hadn't seen that. Well then using it here too makes sense I suppose. Sorry for the noise :)