Bazel: pull in some dependencies to the internal repo #17382
Conversation
This allows to build the ruby and python packs (and the upcoming experimental rust one) from within `codeql`.
|
hmm, this is broken on macOS, I need some more changes |
There was a problem hiding this comment.
Hey,
I have added a bunch of comments, all around the same topic. Let's discuss those further, and once we have agreed on a direction, I'll review the internal PR.
I also raised a discussion on slack about integration testing build system changes in the submodule.
|
|
||
| rust_host_tools = use_extension("@rules_rust//rust:extensions.bzl", "rust_host_tools") | ||
|
|
||
| rust.toolchain( |
There was a problem hiding this comment.
Is this definition really relevant for the external repo? Keeping the versions in sync internally and externally is not-so-great, and toolchains need to be defined in the root module.
In particular, we will never build (release) universal binaries from the public repo, so the extra target triples also are, as far as I can see, unnecessary, if we can avoid triggering the lipo transition.
There was a problem hiding this comment.
I don't think sync would be necessary, but in order to have a lipo rule in this repository one needs the toolchain definition to pull in the additional triples.
Anyway, I'll drop this PR for another approach
| """ | ||
| Checks that the supplied binary doesn't use symbols that are not available in older glibc versions. | ||
| """ | ||
| # Note this accesses system binaries that are not declared anywhere, |
There was a problem hiding this comment.
This can be removed iirc, as rules_python now ships a hermetic python interpreter (verification of that would be nice, though).
| @@ -0,0 +1,66 @@ | |||
| package(default_visibility = ["//visibility:public"]) | |||
There was a problem hiding this comment.
I don't enjoy that we now have the platform definitions externally, but the toolchain defs internally (and the toolchains themselves belong to and need to stay internally AFAIUI).
Is there any way we can put this file back to the internal repo?
There was a problem hiding this comment.
I didn't mind too much, as this is defining constraints, not the actual toolchains. But in any case, this will go
| # this default also covers bazel's own default | ||
| return "@codeql//misc/bazel/platforms:host_toolchain_%s" % arch | ||
|
|
||
| def _x86_32_transition_impl(settings, _attr): |
There was a problem hiding this comment.
This is now a lot of stuff we're only using internally (as we don't have C++ code that ends up in a pack in the external repo). That introduces a coupling between internal+external repo we didn't have before. Would it make sense to split this up into the universal binary transition, and the rest?
I believe we already transitively depend on the apple_support (bazel itself does, iirc). Avoiding that dependency was the only reason not to use https://github.com/bazelbuild/apple_support/blob/master/doc/rules.md#universal_binary.
Maybe now's the time to drop our lipo rule and use that instead? That saves us from having to move some of this code around, and an external dep is always easiest from a repo coordination perspective.
There was a problem hiding this comment.
I believe we already transitively depend on the apple_support (bazel itself does, iirc). Avoiding that dependency was the only reason not to use https://github.com/bazelbuild/apple_support/blob/master/doc/rules.md#universal_binary.
Maybe now's the time to drop our lipo rule and use that instead? That saves us from having to move some of this code around, and an external dep is always easiest from a repo coordination perspective.
I don't think we rely implicitly on apple_support, at least it does not appear in bazel mod graph. The big downside we found in apple_support and its universal_binary, is that it relied on a full Xcode installation (I think it calls xcrun lipo), and we really don't want such a heavy dependency for our developers when we can use command line tools. I don't think that has changed in the meantime.
This is another shot at #17382, using a different and more lightweight approach. This allows building the ruby and python (and in the future also rust) packs from within the codeql repository. This will: * skip defining the glibc symbols checking, which only makes sense when building the release from the internal repository * stub out our `universal_binary` rule, which we only need when building the release.
This allows to build the ruby and python packs (and the upcoming experimental rust one) from within
codeql. The files have been copied without any changes (besides some path changes) in c1fa0e3. Then 2931721 fixes an initial problem with the lipo transition, and restricts it tooptmode: the idea being that we only need universal binaries in the release, but a developer explicitly using thedbgorfastbuildcompilation mode can do away without the double building.A follow-up PR on the internal repo will remove the originals and use the definitions introduced here.