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

bazel: ensure envoy-mobile is able to be built from an external repo #877

Merged
merged 4 commits into from
Jun 9, 2020

Conversation

buildbreaker
Copy link

@buildbreaker buildbreaker commented Jun 4, 2020

  1. Add @envoy_mobile to loading bazel files which live in envoy mobile
  2. Clean up the aar_with_jni macro and include sources and javadoc generation as part of the macro
  3. Ensure we use java8 for executing mac os (dokka requires java8 rather than java14 which is what is used by default)

Signed-off-by: Alan Chiu achiu@lyft.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: bazel: ensure envoy-mobile is able to be built from an external repo
Risk Level: low
Testing: ci + local
Docs Changes: n/a
Release Notes: n/a
[Optional Fixes #Issue]
[Optional Deprecated:]

@buildbreaker buildbreaker force-pushed the bazel-ensure-envoy_mobile-references branch 4 times, most recently from 4a0320b to c5f7ce1 Compare June 7, 2020 02:15
Signed-off-by: Alan Chiu <achiu@lyft.com>
@buildbreaker buildbreaker force-pushed the bazel-ensure-envoy_mobile-references branch from b23d753 to 622a173 Compare June 8, 2020 17:06
@buildbreaker
Copy link
Author

👀 @keith -- most of this is pretty vanilla: find+replace or some small tweak to a target.

The main/big change is the bazel/aar_with_jni.bzl file which I hope organizes things a little better

@buildbreaker buildbreaker requested a review from keith June 8, 2020 19:07
@junr03
Copy link
Member

junr03 commented Jun 8, 2020

1. Add `@envoy_mobile` to all absolute bazel references (target dependencies which start with `//`)

Can you explain why this is needed? I have not seen targets pertaining to a workspace need to reference the project with an @project prefix before. For instance, envoy does not have this.

stamp = True,
tools = ["@envoy_mobile//bazel:zipper"],
visibility = ["//visibility:public"],
Copy link
Member

Choose a reason for hiding this comment

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

Is an aar not already output as a zip? if it is you could probably remove this

BUILD Show resolved Hide resolved
touch $@
""",
outs = ["envoy_mobile.zip"],
cmd = "$(location @envoy_mobile//bazel:zipper) fc $@ $(SRCS)",
stamp = True,
Copy link
Member

Choose a reason for hiding this comment

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

Seems like you probably shouldn't need stamp here 🤔

Copy link
Author

Choose a reason for hiding this comment

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

What does that do actually?

bazel/BUILD Show resolved Hide resolved
Comment on lines +156 to +157
echo "No jni directory found"
fi
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo "No jni directory found"
fi
echo "No jni directory found"
exit 1
fi

might want to fail here

Copy link
Author

Choose a reason for hiding this comment

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

It was to allow for the optionality of not having a native dependency (helped with testing this bazel rule)

cp $$original_directory/$$src_proguard_txt ./proguard.txt
cp $$original_directory/$$src_manifest_xml AndroidManifest.xml
zip -r tmp.aar * > /dev/null
cp tmp.aar $$original_directory/$@
Copy link
Member

Choose a reason for hiding this comment

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

at some point with these scripts you might be better off splitting them into separate files

Copy link
Author

Choose a reason for hiding this comment

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

Would it be better to split each private method into a separate file now?

keith
keith previously approved these changes Jun 9, 2020
@buildbreaker
Copy link
Author

@junr03

Can you explain why this is needed? I have not seen targets pertaining to a workspace need to reference the project with an @project prefix before. For instance, envoy does not have this.

I think I misunderstood a bazel error and I'll need to load the appropriate rules. Let me fix this up.

Signed-off-by: Alan Chiu <achiu@lyft.com>
Alan Chiu added 2 commits June 8, 2020 18:39
Signed-off-by: Alan Chiu <achiu@lyft.com>
fix
Signed-off-by: Alan Chiu <achiu@lyft.com>
Copy link
Member

@junr03 junr03 left a comment

Choose a reason for hiding this comment

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

lgtm, do you mind updating the title, as this PR does more than that?

@buildbreaker buildbreaker changed the title bazel: ensure @envoy_mobile references bazel: ensure envoy-mobile is able to be built from an external repo Jun 9, 2020
@buildbreaker buildbreaker merged commit 57cb7fd into master Jun 9, 2020
@buildbreaker buildbreaker deleted the bazel-ensure-envoy_mobile-references branch June 9, 2020 21:44
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.

3 participants