Delay connecting to the remote executor until executorInit

[jmmv]

This change moves the majority of the code in the beforeCommand hook of the RemoteModule to executorInit. This makes the former validate the configuration flags only, and postpones any attempts to connect to the remote executor until it's actually needed.

The reason for delaying the connection to the executor is to ensure that commands that do /not/ require it can run through completion without errors nor pauses.

To make the diff of this change small, I've tried to leave the code where it was by wrapping it in a new function that gets called from executorInit. The indirection is not necessary for anything else though.

Fixes #18760.

[coeuvre]

I like the way you made the diff small, so clean! Please fix failing tests and we are ready to go.

[jmmv]

Oops, turns out that while the original "simple" change mostly worked... it boke various tests. I was able to fix some a couple of weeks ago but encountered others that were trickier, and lost the chance to continue looking.

Let me move this to a draft PR in the meantime until I have a chance to address the failures.

... but feel free to take it over if you wish :P

[jmmv]

Alright. I finally had a chance to get back to this. It was slightly more complicated than anticipated due to all of the side-effects that the RemoteModule lifecycle has... but all tests are passing now.

[coeuvre]

Thanks!

[coeuvre]

Somehow we need to prioritize #18607, i.e. we want to make handshake parallel to analyze phase. I need to hold the merge of this PR until I have a clear solution for both #18607 and #18760. Sorry.

[jmmv]

I don't have the full picture, but I think that to fix #18607, you'll have to start by resolving the same issues that this PR addresses, so it may be beneficial to get it in. RemoteModule initialization is too complex due to its side-effects and hidden dependencies on global state, so initializing it in the background is going to hit the same problems I found. In other words: this PR already resolves the issue of delaying initialization until later but still happens synchronously. If you want initialization to happen in the background, wrapping the new executorInit logic into a future should be easier than trying to deal with what existed before.

Separate question: is there a plan to fix this all for Bazel 7, possibly due to Skymeld? The reason I ask is because this patch is intrusive, so I'm debating whether it makes sense for me to maintain it as an internal patch. If there is a clear path towards deleting it altogether (because it will be integrated "as is" into Bazel or because it will be rewritten in some other way), then the risk is low. But if not... I'll face trouble updating it later on :P

[coeuvre]

I don't have the full picture, but I think that to fix #18607, you'll have to start by resolving the same issues that this PR addresses, so it may be beneficial to get it in. RemoteModule initialization is too complex due to its side-effects and hidden dependencies on global state, so initializing it in the background is going to hit the same problems I found. In other words: this PR already resolves the issue of delaying initialization until later but still happens synchronously. If you want initialization to happen in the background, wrapping the new executorInit logic into a future should be easier than trying to deal with what existed before.

I agree the initialization is too complex especially when other modules rely on the initialization result. What this PR didn't address are:

1. Properly initialize remote module for BEP uploader and remote downloader.
2. While we can delay / move the initialization code as is, other modules still call / dependent it in beforeCommand. So it doesn't resolve the problem fundamentally.

The solution I am looking for is probably store a future of channel in RemoteCache / other similar places while it can still correctly propagate the initialization errors.

Separate question: is there a plan to fix this all for Bazel 7, possibly due to Skymeld? The reason I ask is because this patch is intrusive, so I'm debating whether it makes sense for me to maintain it as an internal patch. If there is a clear path towards deleting it altogether (because it will be integrated "as is" into Bazel or because it will be rewritten in some other way), then the risk is low. But if not... I'll face trouble updating it later on :P

I am working on the fix now. I expect to resolve this within this week unless something changed.

[coeuvre]

Closing, in favor of #18607.