Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow code generation without android SDK/NDK #154

Merged
merged 1 commit into from
Nov 22, 2023

Conversation

sonicdebris
Copy link
Contributor

@sonicdebris sonicdebris commented Nov 21, 2023

We have the same issue mentioned in #107 (specifically: #107 (comment)).

In short, some of our developers working with the generator are not android developers, and would rather not have to set-up the android SDK/NDK to use djinni, considering those are only needed to build the android examples.

I know nothing about Bazel, but still I decided to try and find a solution to this. I started investigating a way to conditionally trigger the android repo rules for the examples only, when I found this discussion:
bazelbuild/bazel#14260

From there, I understand that the rule should only be applied in case one tries to build a target that requires the android SDK, but apparently that is not the case yet. In the same discussion, I found a suggestion for a workaround, which amounts to only defining the android_sdk_repository and android_sdk_repository rules if the environment variables for the paths to the SDK/NDK are set:
envoyproxy/envoy-mobile#2039

I implemented the same, with a trivial tweak, which is printing a warning in case the environment variables are not set, to counterbalance the fact that the build failure message for the android-app and android_sdk_repository is now less explicit.

It seems to work fine, hopefully it's not too hacky and can be useful upstream.

@li-feng-sc
Copy link
Contributor

Thanks! This is something I have wanted for a long time. I'll need to do some tests with Snap's codebase and then get back to you.

@li-feng-sc
Copy link
Contributor

@sheldonneuberger-sc we use a trimmed down version of the workspace file, so I believe this will not impact Snap code.

@li-feng-sc
Copy link
Contributor

I tested this on my laptop and it works very well! Thank you @sonicdebris !

Copy link
Contributor

@sheldonneuberger-sc sheldonneuberger-sc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thanks for fixing that.

@li-feng-sc li-feng-sc merged commit c7b0a39 into Snapchat:main Nov 22, 2023
1 check passed
@nmtitov
Copy link
Contributor

nmtitov commented Nov 22, 2023

This is very good idea, I like it. I mostly do iOS development and having Android dependency installed is really annoying for me.

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