build: remove dist/

[keith]

Previously the dist/ contained archives built for either platform, that
were then included by other targets to be used in the example apps. This
had the benefit of demonstrating how the targets could be used in a
bazel setup. The downside with this approach is that it was required to
be non-hermetic, and made builds a 2 step process that you had to go
through if you wanted to test framework changes. This is now removed and
replaced with a more standard bazel setup where the example targets
reference other targets in the build, removing the need for 2 steps.
This also eliminates issues caused by manually editing the files in the
dist/ directory, which bazel wouldn't pick up since it was non-hermetic.
For android this is a no-op because we still continue to include the aar
directly, for iOS this isn't currently possible because the
apple_static_framework_import rule requires a list of files, which
isn't possible to compute from the zip file produced by the
ios_static_framework rule that is only meant for distribution. Instead
the examples now depend on the swift_library directly, which is
slightly different, but shouldn't be different enough to cause
significant issues.

Signed-off-by: Keith Smiley keithbsmiley@gmail.com

[jpsim]

Great simplification, thanks!

[jpsim]

What does this do?

[keith]

Yea so if you use bazel transitions, which the apple rules do, you get some path like bazel-out/foo/bar-RANDOMJUNK/path/to/artifact, with this flag it also creates the usual bazel-bin/path/to/artifact. It has one limitation that it doesn't work through alias targets bazelbuild/bazel#14602 so in some cases on CI I use the real target name. Locally this shouldn't matter much since folks will just copy paste, but on CI it makes it stable

[goaway]

Thanks @keith, appreciate you work to explore alternatives here. As it stands, I'm not totally sold on the current set of proposed changes in this PR.

The dist/ setup was part of a deliberate decision we made early on to ensure the example apps did not utilize Bazel targets and did leverage the artifacts we expect most users of the library to consume - the .aar and the .framework.

While I agree that having the non-hermetic dist/ directory runs counter to general bazel philosphy - and this is unfortunate - I still feel that having the example apps rely on internal targets makes them less generally helpful for the average potential user, since we're no longer demonstrating how the distributable packages can be used.

Arguably, we could set up the examples as even more traditional Android and iOS projects that don't rely on Bazel at all, as most folks are likely not using Bazel to build their app.

Would also be interested in discussing other possibilities.

[goaway]

The other piece to this, that is perhaps even more significant, is that having the test apps depend on the distributable packages allows us to actually test the viability of the packages in CI. We'd miss that coverage if we switched exclusively to internal bazel targets for tests.

[jpsim]

I still feel that having the example apps rely on internal targets makes them less generally helpful for the average potential user, since we're no longer demonstrating how the distributable packages can be used.

Arguably, we could set up the examples as even more traditional Android and iOS projects that don't rely on Bazel at all, as most folks are likely not using Bazel to build their app.

This is exactly the point I was going to make, we can and should have validation on CI for all our supported installation methods, I'm happy to spend time setting up that infra. Keeping the bazel parts hermetic is a big win.

[keith]

I pushed an update which allows us to import the framework directly just as before with the help of a small rule. This should negate any concerns about this behavior being different than dist/ before

[goaway]

Thanks @keith, your update definitely helps to allay my concern about the CI coverage aspect.

Out of curiosity, if we were to move in the direction of setting up example apps specifically as more traditional Xcode/Android Studio projects (i.e. without Bazel, necessarily), do you have any ideas for what that might look like with this setup?

[jpsim]

Out of curiosity, if we were to move in the direction of setting up example apps specifically as more traditional Xcode/Android Studio projects (i.e. without Bazel, necessarily), do you have any ideas for what that might look like with this setup?

A vanilla Xcode project could refer to the .framework directly in bazel-bin/. I imagine the equivalent on Android would be similar, where the Android Studio project configuration could point to the .aar in bazel-bin/ too.

[jpsim]

+1

Some notes:

1. This reorganizes the Android/iOS example apps CI jobs from being an initial step to build Envoy Mobile, uploading the release artifact as a GitHub Actions artifact, the spawning multiple jobs for each example app that downloads the built Envoy Mobile. The new format builds and runs all example apps in parallel, which will shorten overall CI time, but won't benefit from reusing a single cached built artifact, but with bazel's reproducibility and caching features, this shouldn't really be a problem.

2. We should invest in a validation suite that integrates Envoy Mobile in all our supported installation methods the same way a user would integrate it. The framework_imports_extractor is an impressive trick to get close to that on the iOS front, but does not really reflect how our users would integrate the framework in Xcode without bazel, and doesn't validate our other supported installation methods.

[jpsim]

Can remove dist completely:

Suggested change

	--add-excluded-prefixes ./envoy/ ./envoy_build_config/extensions_build_config.bzl ./WORKSPACE ./dist/ ./library/common/config_template.cc ./bazel/envoy_mobile_swift_bazel_support.bzl ./bazel/envoy_mobile_repositories.bzl \
	--add-excluded-prefixes ./envoy/ ./envoy_build_config/extensions_build_config.bzl ./WORKSPACE ./library/common/config_template.cc ./bazel/envoy_mobile_swift_bazel_support.bzl ./bazel/envoy_mobile_repositories.bzl \

Same with the line a bit further down.

[keith]

I was worried about removing this while folks still have this ignored directory locally. maybe we wait and delete in the future?

[goaway]

1. This reorganizes the Android/iOS example apps CI jobs from being an initial step to build Envoy Mobile, uploading the release artifact as a GitHub Actions artifact, the spawning multiple jobs for each example app that downloads the built Envoy Mobile. The new format builds and runs all example apps in parallel, which will shorten overall CI time, but won't benefit from reusing a single cached built artifact, but with bazel's reproducibility and caching features, this shouldn't really be a problem.

This was actually substantial performance improvement O(hours) to CI at least prior to having RBE. The app tests are somewhat more prone to flakiness due to network faults and the like, and previously having to rebuild the entire library to rerun them was extremely painful. Do we have RBE caching effectively across runs?

2. We should invest in a validation suite that integrates Envoy Mobile in all our supported installation methods the same way a user would integrate it. The framework_imports_extractor is an impressive trick to get close to that on the iOS front, but does not really reflect how our users would integrate the framework in Xcode without bazel, and doesn't validate our other supported installation methods.

This was the essence of my question above - I feel the example apps are if anything moving away from being representative of an "example" of how to use Envoy Mobile from an average user's perspective. To your response @jpsim, are the .framework and .aar output to stable directories inside bazel-bin?

[jpsim]

This was actually substantial performance improvement O(hours) to CI at least prior to having RBE. The app tests are somewhat more prone to flakiness due to network faults and the like, and previously having to rebuild the entire library to rerun them was extremely painful. Do we have RBE caching effectively across runs?

Yes, the RBE cache should be pretty effective, and in fact probably even more now that we've removed the non-cached, non-hermetic artifacts in dist/. I also have seen that uploading and downloading built artifacts from GitHub Actions was pretty slow. We may get a speed up from the bazel cache and executors all being in EngFlow's network.

As a point of comparison, android_build took 6 minutes and java_helloworld took 8 minutes in this PR, compared to 9 minutes + 12 minutes in #2261, so from commit to job complete, java_helloworld took 8 minutes in this PR vs 21 minutes in #2261.

This was the essence of my question above - I feel the example apps are if anything moving away from being representative of an "example" of how to use Envoy Mobile from an average user's perspective. To your response @jpsim, are the .framework and .aar output to stable directories inside bazel-bin?

Yes, the .framework and .aar artifacts are output to stable locations with this PR, they just happen to be in bazel-bin/ instead of dist/. My point was actually that neither what we were doing before, nor with this PR, do we have a representative setup of how most users will integrate Envoy Mobile, and that it would be worth us investing in a validation suite that confirms that all our supported installation methods work: bazel, .framework/.aar, Maven, CocoaPods & SwiftPM. On main today and with this PR, we have a mixed bazel/binary validation that in practice none of our users are likely to have.

[goaway]

Thanks for the discussion @keith and @jpsim.

Approving this, but do want to note that it was historically convenient to have code at the ready that could be run on top of the real distributable artifact, and I would like us to work towards finding a solution that allows for that again.

[jpsim]

have code at the ready that could be run on top of the real distributable artifact

With this change, the app code still runs on top of the real distributable artifact, but what you shared offline is something slightly different, and still valuable, which is the ability to more easily build the framework and the app with different build settings. As it stands now, with building the app also building the framework, those build settings propagate unless you go add explicit setting overrides to where the targets are defined.

I think the building an installation method validation suite I've suggested here would achieve that.