Add a persistent worker to support swiftc incremental compilation.
#174
Conversation
| // declaration model does not allow this. | ||
| // | ||
| // One could disable the sandbox to hack around this, but this should not be a | ||
| // requirement of a well-designed build rule implementation. |
There was a problem hiding this comment.
We've discussed this before, but it has never gone beyond labeling ("hack", "well designed", etc). I really want to understand this. Are there documents or discussions somewhere that we can link to that discuss real pros and cons? Perhaps there is info that googlers are exposed to that the community is not, because my outsider perspectives (takes) are:
- persistent workers are themselves a hack, but since they're blessed there's an implicit appeal to authority, or something
- adding a "no-sandbox" requirement to just those actions that need incremental appears to achieve the exact same level of escape, but is simpler, doesn't bring in all the accidental complexity of persistent workers
- persistent workers don't model persistent files (adding persistent processes where they aren't required for anything)
I could go on, but I mainly just want to highlight that I feel a misunderstanding. I've been trying to see it from the other angle, but I can't, and when I've asked questions, I still come up empty. I feel no-sandbox is objectively better in every way, why is that you don't? 😂
There was a problem hiding this comment.
Disabling the sandbox is using a big hammer to hit a very specific nail, and it's not something that should be done lightly because it can lead to correctness issues. In the past, we've seen incorrect Objective-C builds due to sandboxing differences in local and remote builds—a local build (with sandboxing disabled) would import headers that were in build targets that it forgot to depend on, and then the corresponding remote build (with sandboxing enabled) would break post-submit because the forgotten dependency meant the headers wouldn't be pulled into the remote sandbox.
That particular situation is less of an issue for Swift because of its dependency model, but as a general rule of thumb disabling the sandbox should be considered a "last resort" (for example, rules_apple does this for ibtool/actool actions that act up with sandboxing enabled) but not as a way to accomplish something that has other means that have been designed for that purpose. The implementation of the persistent worker provides better control over what's going on than just "let this action access anywhere it wants."
I also have plans to get rid of the bazel_xcode and wrapped_swift wrappers and merge everything into the worker, using it as a sort of "überdriver" for all Swift actions—this will greatly simplify the action registration logic in the rules. (Not that that will affect end users that much, but it's a tech debt reduction I'm looking forward to.)
|
|
||
| if (!is_wmo) { | ||
| // Copy the output files from the incremental storage area back to the | ||
| // locations where Bazel declared the files. |
There was a problem hiding this comment.
Also planning to look at CoW (on macOS) or hard linking as possible optimizations here, as long as that doesn't interfere with Bazel in weird ways.
tools/worker/work_processor.cc
Outdated
|
|
||
| // Pass the incremental flags regardless of whether WMO is enabled or not. | ||
| // The driver will prefer WMO over incremental, but by passing this flag, we | ||
| // don't silently swallow a warning from the compiler that incremental mode |
There was a problem hiding this comment.
I think for us we would rather silence this worker. My thinking with our prototype worker was that we would always pass --strategy=SwiftCompile=worker and then in some cases optionally pass wmo. Reason being I'd rather always go through the persistent worker codepaths, or never go through it, since I think that would reduce the surface area of places we have to debug when something goes wrong.
There was a problem hiding this comment.
Yeah, that's a good point. In some cases (for example, -wmo on individual targets), I think it makes sense to keep the warning because one day I hope people won't be setting that manually, but that's overshadowed by the fact that you'd have to switch strategies just to do an optimized build or you'd get a ton of noise in your build.
There was a problem hiding this comment.
Yea, another case that @kastiglione brought up is that depending on the target wmo builds may be faster than incremental builds, or you may want to do wmo builds for some targets just because they user doesn't care about having incremental builds for them, in those cases I think you'd also like to not have the warning
There was a problem hiding this comment.
I just added -whole-module-optimization to an Xcode invocation using -incremental, and there was no warning. Also just scrum swiftc -incremental -whole-module-optimization ... *.swift didn't produce a warning.
There was a problem hiding this comment.
Ah-ha, I see why. It only shows up when you have -debug-show-incremental enabled; I thought it was present unconditionally.
swiple-rules-gardener
force-pushed
the
test_242204312
branch
3 times, most recently
from
0ea0117
to
5ad36ea
Compare
To perform incremental builds, pass the `--strategy=SwiftCompile=worker` flag to Bazel (or add it to your .bazelrc file). RELNOTES: Incremental compilation is now supported by passing the `--strategy=SwiftCompile=worker` flag to Bazel. PiperOrigin-RevId: 242511066
swiple-rules-gardener
force-pushed
the
test_242204312
branch
from
5ad36ea
to
c197356
Compare
Add a persistent worker to support
swiftcincremental compilation.To perform incremental builds, pass the
--strategy=SwiftCompile=workerflag to Bazel (or add it to your .bazelrc file).Fixes #48.