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

Cannot include generated file as a resource #281

Closed
rockwotj opened this issue Feb 17, 2020 · 8 comments · Fixed by #538
Closed

Cannot include generated file as a resource #281

rockwotj opened this issue Feb 17, 2020 · 8 comments · Fixed by #538

Comments

@rockwotj
Copy link
Contributor

Using the latest commit from master, this fails:

genrule(
    name = "generated_logging_properties",
    srcs = [":local.logging.properties"],
    outs = ["logging.properties"],
    cmd = "cp $< $@",
)

kt_jvm_library(
    name = "logging",
    srcs = glob(["*.kt"]),
    resources = [":generated_logging_properties"],
    visibility = ["//visibility:public"],
    deps = [
        "@maven//:com_google_flogger_flogger",
        "@maven//:com_google_guava_guava",
        "@maven//:com_google_flogger_flogger_system_backend",
        "@maven//:org_slf4j_slf4j_jdk14",
    ],
)
ERROR: /Users/rockwotj/Workspace/foo/kotlin/logging/BUILD:10:1: error executing shell command: '/bin/bash -c external/bazel_tools/tools/zip/zipper/zipper c bazel-out/darwin-fastbuild/bin/kotlin/logging/logging-resources.jar @bazel-out/darwin-fastbuild/bin/kotlin/logging/logg...' failed (Exit 255) bash failed: error executing command 
  (cd /private/var/tmp/_bazel_rockwotj/70f6e107f8f90b5b2d301b4029134ae9/sandbox/darwin-sandbox/12/execroot/__main__ && \
  exec env - \
  /bin/bash -c 'external/bazel_tools/tools/zip/zipper/zipper c bazel-out/darwin-fastbuild/bin/kotlin/logging/logging-resources.jar @bazel-out/darwin-fastbuild/bin/kotlin/logging/logging_resources_zipper_args')
Execution platform: @local_config_platform//:host

Use --sandbox_debug to see verbose messages from the sandbox
File kotlin/logging/logging.properties does not seem to exist.Target //kotlin/logging:logging failed to build

@restingbull
Copy link
Collaborator

Try compiling //kotlin/logging:generated_logging_properties directly. It looks like the genrule is not building.

@rockwotj
Copy link
Contributor Author

rockwotj commented Feb 26, 2020

@restingbull the genrule does build correctly and it's output is in bazel-bin

@restingbull
Copy link
Collaborator

Investingating

@rockwotj
Copy link
Contributor Author

If it would help I can create a small sample repo demoing this behavior

@rockwotj
Copy link
Contributor Author

Looks related: #187

@sugarmanz
Copy link
Contributor

sugarmanz commented Jul 30, 2020

I'm encountering the same issue, which is preventing adoption of Bazel for this usecase in a polyglot project. Is there any update here @restingbull, @rockwotj, @cgruber?

@rockwotj
Copy link
Contributor Author

@sugarmanz you can work around this by wrapping it in a Java library rule that is a dep

@sugarmanz
Copy link
Contributor

sugarmanz commented Jul 31, 2020

Ahh, thanks for the insight there. I'll try that out.

BenHenning added a commit to oppia/oppia-android that referenced this issue Jun 12, 2024
## Explanation
Fixes part of #1719 and #3709

This PR introduces a new script & CI workflow for computing build stats
to compare both AABs and universal APKs between develop and the changes
in a given PR, as part of fixing #1719 (though this PR doesn't cover
everything outlined in that PR). This information is then detailed and
uploaded as a CI build artifact, and summarized & posted as a comment in
the PR. Some details included in the summary report:
- APK file/download size differences
- Method count differences
- Feature/permission differences
- New/removed resources & assets

The script supports computing differences for multiple "profiles" at the
same time, and the CI workflow has been set up to compute four:
1. dev
2. alpha
3. beta
4. GA

This workflow will be optional since it's very expensive to run (it has
to assemble 8 builds, 6 of which are Proguarded). It also doesn't really
need to be run in order to approve a PR, though reviewers may insist on
waiting for large or suspicious changes
(such as PRs introducing new dependencies) to ensure the actual affected
changes are as expected.

In order to mitigate this expense, the CI workflow runs on a scheduled
cron job off of develop across all open PRs and checks them in a group.
It runs at most once per day (based on
https://github.com/orgs/community/discussions/55768#discussioncomment-5941720)
so multiple changes to a PR will be picked up with a single check in the
next cron run. Currently, it will run even for a PR that hasn't changed
since the last run (but this is something that can be improved in the
future if it needs to be). It's being scheduled for 2:30am (02:30) UTC
which seems to have a few specific benefits:
- Per GitHub documentation, initiating the workflow outside the start of
the hour should reduce likelihood of cancellation (since the start of
the hour tends to use the most resources):
https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#schedule.
- This corresponds to 7:30pm PT, 2:30am GMT, 8:00am IST, 5:30am EAT, and
12:30pm AEST (just as a basis for a different part of the world). It's
actually a very nice time that shouldn't overlap with almost any
development in main locations around the world, so it hopefully won't
impact Oppia organization GitHub CI resources.

The example output from these workflows can be observed in a few places:
- Later in this PR (for back when the PR was configured to run the new
workflow per PR change).
- In #4261 which demonstrates the large math PRs and how they changed
the builds back before those were merged.
-
https://github.com/BenHenning/oppia-android/actions/workflows/stats.yml
and https://github.com/BenHenning/oppia-android/pulls (specifically:
BenHenning#14,
BenHenning#13,
BenHenning#12) which demonstrates
the workflow running correctly from a scheduled cron
(https://github.com/BenHenning/oppia-android/actions/runs/9232187176)
and posting the updates to open PRs.

Beyond that, implementing this utility involved several significant
changes to various systems, including the build graph:
- Three new utilities were added for the script: Aapt2Client,
ApkAnalyzerClient, and BundleToolClient. Each of these correspond to
Android CLI utilities, but each required special considerations:
- Aapt2Client requires direct access to the Android SDK, but fortunately
android_sdk_repository exposes this as a target so it's trivial to pass
it in & call it. Some build information is needed, too (see next outer
point).
- ApkAnalyzerClient couldn't use the apkanalyzer CLI contained within
the SDK since it's not exported by android_sdk_repository. Instead, we
needed to depend on the CLI's internal implementation library (which I
suspect is what Android Studio probably uses for its own APK Analyzer
tool). This required some new implementation.
- BundleToolClient fortunately can call right into the bundle tool
library that we use when building AABs, but unfortunately that tool
appears to not be designed to be called multiple times in the same
process. Because Java doesn't support forking, we actually needed to
fake a fork function by starting a new Java process using the current
process's classpath in order to re-run bundle tool for each needed
routine. Additionally, bundle tool required
https://github.com/oppia/archive-patcher (which needed new BUILD files
since it only supported Gradle building previously) and a non-Android
version of Guava (see below for the changes this has caused).
- A new build_vars.bzl was introduced to define the build SDK & build
tools versions (this is done in a way where they can actually be passed
to the new script's utilities since it needs to access aapt2).
- rules_kotlin had a bug where resources wouldn't be pulled in properly
for kt_jvm_library (see
bazelbuild/rules_kotlin#281), but this was
mitigated in a previous PR by upgrading rules_kotlin past alpha 2.
- The new functionality required the JRE-compatible version of Guava
(over the Android-constrained library used in the codebase today), but
this introduces a one-version issue. The solution ended up being
isolating the JRE-compatible Guava library to its own library with a
slightly hacky direct reference to it in BundleToolClient. Some of the
other attempts at solving this resulted in some Maven reference cleanups
in existing script documentation. This functionality will be improved in
downstream PRs, but other attempts that were originally made to isolate
this cleanly were:
- Introduce multiple maven_install files and isolate dependencies into:
production, tests, scripts. This has a number of nice benefits (more
correct licenses and faster Maven dependency fetches for production),
but it results in very tricky one-version violations for test targets
that cross dependencies between production and tests.
- Isolated maven_install just for scripts. This is closer to the
solution we'll want long-term, but it was too much complexity to fully
introduce in this PR so it's been reworked into a downstream PR that can
focus on cleaning up third-party dependency management across the whole
codebase.

This PR is introducing a few new dependencies that, in turn, pull in a
*bunch* of transitive dependencies. These are all due to the new
``apkanalyzer`` dependency. While it will affect licenses for this
specific PR, once third-party dependencies for scripts are cleaned up in
a downstream PR they will be moved out (since they are script-only
dependencies).

Separately, also note that the AAPT2 utility requires stdout to be
processed continuously in order for the process to finish. This was one
of the primary reasons CommandExecutorImpl was reworked in #4929.

For testing: most of the changes in this PR have been extensively
manually tested. However, the new utilities are lacking significant
automated tests. Since this utility is a nice-to-have for the rest of
the Bazel PR chain, it's being prioritized to be merged in spite of
lacking code coverage. #4971 has been filed to track adding these
missing tests in the long-term.

## Essential Checklist
- [x] The PR title and explanation each start with "Fix #bugnum: " (If
this PR fixes part of an issue, prefix the title with "Fix part of
#bugnum: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## For UI-specific PRs only
N/A -- This only affects CI workflows & the build system. Technically,
some dependency changes in the build system could have UI effects, but
there should be no such changes in this PR.

---------

Co-authored-by: Adhiambo Peres <[email protected]>
Co-authored-by: Sean Lip <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants