Skip to content

RFC: Add Bazel build configurations for Apollo iOS#1309

Closed
liuliu wants to merge 1 commit intoapollographql:mainfrom
liuliu:lliu-bazel-apollo
Closed

RFC: Add Bazel build configurations for Apollo iOS#1309
liuliu wants to merge 1 commit intoapollographql:mainfrom
liuliu:lliu-bazel-apollo

Conversation

@liuliu
Copy link
Copy Markdown
Contributor

@liuliu liuliu commented Jul 10, 2020

I plan to add some Bazel support for Apollo iOS, including runtime BUILD
files and some bzl macros that can download schema and generate code
from the schema.

Before that, this PR added relevant BUILD configuration to build Apollo
/ ApolloCore / ApolloWebSocket / ApolloSQLite.

I am not sure what SwiftScripts do, and whether these will work as
expected, since Bazel sandbox the binary therefore the path finding
logic will be incorrect.

For the codegen part, I need to figure out a few things:

  1. It seems OK to wrap the node / run with sh_binary, with additional
    shell scripts (run-bundled-codegen.sh messes with Bazel sandbox and
    I am not happy to just wrap that, mainly because it won't be
    properly cached (the downloaded tar.gz file)).

  2. I need to use http_archive most likely for the apollo cli tools (in
    node.js). However, the current link doesn't end with tar.gz, which
    Bazel doesn't like.

Hence, this PR upstreams some conservative changes. Please let me know
what you maintainers think.

The broad picture is to make integration with Bazel based iOS project
easier, and uses Bazel's cache for schema download, code generation etc.

I plan to add some Bazel support for Apollo iOS, including runtime BUILD
files and some bzl macros that can download schema and generate code
from the schema.

Before that, this PR added relevant BUILD configuration to build Apollo
/ ApolloCore / ApolloWebSocket / ApolloSQLite.

I am not sure what SwiftScripts do, and whether these will work as
expected, since Bazel sandbox the binary therefore the path finding
logic will be incorrect.

For the codegen part, I need to figure out a few things:

 1. It seems OK to wrap the node / run with sh_binary, with additional
    shell scripts (run-bundled-codegen.sh messes with Bazel sandbox and
    I am not happy to just wrap that, mainly because it won't be
    properly cached (the downloaded tar.gz file)).

 2. I need to use http_archive most likely for the apollo cli tools (in
    node.js). However, the current link doesn't end with tar.gz, which
    Bazel doesn't like.

Hence, this PR upstreams some conservative changes. Please let me know
what you maintainers think.

The broad picture is to make integration with Bazel based iOS project
easier, and uses Bazel's cache for schema download, code generation etc.
@apollo-cla
Copy link
Copy Markdown

@liuliu: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

@WALLOUD
Copy link
Copy Markdown

WALLOUD commented Jul 11, 2020

@designatednerd
Copy link
Copy Markdown
Contributor

Interesting! I do have serious concerns about upstreaming this, though:

  • This adds a bunch of untyped config files to the root of the repo with zero context (It'd help if they were in a single folder with a README explaining what the hell Bazel is and what these are for, but I don't know if that's possible), which is confusing for users who don't use Bazel
  • This could cause conflicts for projects who have their own Bazel files already set up already.
  • This is the main one: Adding this to the main fork would mean Apollo would need to support this build option in the long term, and frankly I'm kind of losing it supporting three dependency managers already 🙃
    • I'd actually have to learn how Bazel works, which I do not already know, I just have a general understanding of what it is and why people want to use it
    • Any time we updated dependencies this would add yet another place where and format in which they'd need to be dealt with
    • We would have to set up an entire Bazel build setup for our CI in order to validate that this doesn't break, which appears to be a non-trivial amount of work.
    • We would need to create documentation explaining how this works.

Personally, I'm super-reluctant to upstream this given the additional maintenance overhead vs. the considerably smaller number of users who use Bazel vs other package managers + the normal Xcode build system.

To be honest, I've also generally found that people who have a project big enough to make switching to the Bazel build system worth it generally are at very, very large companies. These projects therefore have access to very talented build engineers who a) can absolutely do this work themselves and b) want to do it Very Specifically Their Way, which having a default setup would likely interfere with.

Given all that, do you want to continue with this PR?

@liuliu
Copy link
Copy Markdown
Contributor Author

liuliu commented Jul 11, 2020

@designatednerd, that's a very reasonable reasoning!

I am putting up the PR to get feedback on:

  1. Whether someone has already done this work, and if I am on the right track;
  2. Get a sense on what the dependencies this project looks like and scope the work properly.

The main motivation is to exploit buck cache for ApolloCLI outputs (schema downloading / code gen).

It seems that:

  1. In the wild, if there are work done, it is probably done internally;
  2. ApolloCLI seems have no dependency on Apollo-iOS, which makes the codegen wrapper easier;
  3. Initially looks large, but current Apollo-iOS has a shallow dependency tree, makes custom runtime builds easier.

There are still some questions I need some answers:

  1. Whether there are plans to move codegen to Swift? I am confused by the ApolloCodegenLib and the node.js based ApolloCLI doesn't look like have any dependencies on that yet;

We can certainly just revisit this once Bazel acquired bigger mind-share. It won't impede my work, I will simply maintain it in a fork. Thanks!

@designatednerd
Copy link
Copy Markdown
Contributor

ApolloCLI seems have no dependency on Apollo-iOS, which makes the codegen wrapper easier;

That's correct, it only depends on ApolloCore.

Whether there are plans to move codegen to Swift? I am confused by the ApolloCodegenLib and the node.js based ApolloCLI doesn't look like have any dependencies on that yet;

So the Codegen lib is doing two things:

  1. Making it possible to use Swift scripts to run code generation (while still using the JS codegen under the hood), since they're way easier to customize and debug. This is already shipped.
  2. Actually do the "code generation" part in Swift, and use either existing JS parsing and validation or use shipping-soon Rust parsing and validation. This is currently a little bit blocked by some decisions we have to make around JS vs Rust, but you can see work in progress with Xcode 12 + iOS 14 + Swift 5.3 + associated Updates #1280 and the Swift Codegen Project.

Basically ideally we'll be able to jettison JS entirely, but it's going to be a while before we can do that.

@liuliu
Copy link
Copy Markdown
Contributor Author

liuliu commented Jul 12, 2020

Thanks! That makes a lot of sense.

I initially started because the Codegen part build (with Bazel) is fairly involved. If it was written in Swift, ideally, I would like to have Bazel build the codegen binary and then use the codegen binary for Bazel macro to do the codegen. Right now with the node.js codegen, I can either wrap the whole thing as a shell (it is not ideal to wrap a Swift binary as shell), and use that for codegen or just use node.js wrapper in Bazel to build.

Otherwise, the runtime part is fairly straightforward to maintain as a fork.

I will follow the project and the issue in case the Swift codegen start to move again and I need to do the fairly involved work again.

@designatednerd
Copy link
Copy Markdown
Contributor

The Swift wrapper portion of codegen automatically downloads and executes the necessary JS stuff, without the need for any nonsense with Node directly. I'm not sure how well this will work with sandboxing, buy because the zip file it downloads includes all of node and all dependencies, it might help. Please check out the Swift Scripting docs for more details.

@designatednerd
Copy link
Copy Markdown
Contributor

Thought about it over the weekend, but I'm sticking with my original though about not supporting Bazel officially, for the reasons I've already outlined.

I'm going to go ahead and close this PR since I have no intention of merging it. - if you want to discuss Bazel support further, our Spectrum Chat is a great place for discussion at this time. Thank you for your efforts though!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants