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

Make ManifestMergerAction worker compatible #14427

Conversation

Bencodes
Copy link
Contributor

Calling System#exit kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

@@ -231,11 +231,6 @@ public static void main(String[] args) throws Exception {
// to the expected location of the output.
Files.copy(manifest, options.manifestOutput, StandardCopyOption.REPLACE_EXISTING);
}
} catch (AndroidManifestProcessor.ManifestProcessingException e) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The root Exception can catch everything, minus the System.exit.

Copy link
Contributor

Choose a reason for hiding this comment

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

This was explicitly added to fix #6904. We should not unfix that.

Copy link
Contributor Author

@Bencodes Bencodes Jan 10, 2022

Choose a reason for hiding this comment

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

Thoughts on raising this specific logic up into ResourceProcessorBusyBox where we can log the exception and return the proper exit code inside processRequest?

This would also make the error handing more consistent for any of the other actions that interact with manifest files. DensitySpecificManifestProcessor is another example where a ManifestProcessingException could be thrown, and technically anything that uses AndroidManifest.java can surface this exception as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, this is a bit strange. There are actually two exception classes with this name:
https://github.com/bazelbuild/bazel/blob/09c621e4cf5b968f4c6cdf905ab142d5961f9ddc/src/tools/android/java/com/google/devtools/build/android/ManifestProcessingException.java

public static class ManifestProcessingException extends RuntimeException {

And they're even both used in the same class:


throw new AndroidManifestProcessor.ManifestProcessingException(e);

We should converge onto one to make this less confusing....

Looks like the top-level class is also being used to capture two different kinds of exceptions:

} catch (IOException | XMLStreamException e) {
throw new ManifestProcessingException(e);
}

IOException is one thing, but XMLStreamException might be from badly formed xml...

Either way, if we just throw here, this exception will get caught by the catch (Exception e) in ResourceProcessorBusyBox, and do the same basic logging. So probably don't need to make ResourceProcessorBusyBox aware of lower level exceptions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either way, if we just throw here, this exception will get caught by the catch (Exception e) in ResourceProcessorBusyBox, and do the same basic logging. So probably don't need to make ResourceProcessorBusyBox aware of lower level exceptions.

The generic catch (Exception e) { in ResourceProcessorBusyBox will re-throw the exception after logging the error and bubble that up as an exception coming from main, while catch (AndroidManifestProcessor.ManifestProcessingException e) { gracefully handles the exception with an exit code of 1 being returned directly via the work response (Prior to this PR is was being returned as a System#exit with 1). Is this slight behavioral change fine to merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably isn't a big deal given that this PR wraps all unhandled exceptions with a proper exit code of 1 in order to confirm to WorkRequestHandler.

https://github.com/bazelbuild/bazel/pull/14428/files#diff-6bbb0aaf4cf2d425734021815617e2b6203223abe7dc76ca8030a90e9f043873R213

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@larsrc-google is this in a mergable state so that #14428 is unblocked?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I'm catching up on this thread right now -- I'll probably add in a comment in the ManifestProcessingException case handler to explain why its handler is almost the same as the generic case, minus the logging statement. I think it should be fine -- either we have essentially two debug log lines of the same error message, or we have a slightly funky-looking try/catch block. Cutting down on noise in the console seems preferable in this case.

@meisterT
Copy link
Member

cc @larsrc-google

@Bencodes Bencodes force-pushed the make-manifestmergeraction-worker-compatible branch from ef047f4 to ab51ff1 Compare January 10, 2022 19:32
@gregestren gregestren added the team-Performance Issues for Performance teams label Feb 3, 2022
@Bencodes Bencodes force-pushed the make-manifestmergeraction-worker-compatible branch from 7654a20 to f3f72d5 Compare February 6, 2022 17:34
bazel-io pushed a commit that referenced this pull request Feb 8, 2022
`ManifestProcessingException` was duplicated somehow and can be removed to avoid confusion.

More context: #14427 (comment)

Closes #14613.

PiperOrigin-RevId: 427074145
@Bencodes Bencodes force-pushed the make-manifestmergeraction-worker-compatible branch 2 times, most recently from 23102bd to 73d3414 Compare February 15, 2022 05:06
@Bencodes Bencodes force-pushed the make-manifestmergeraction-worker-compatible branch from 73d3414 to ab345c7 Compare April 5, 2022 19:46
@Bencodes Bencodes requested a review from ted-xie as a code owner April 5, 2022 19:46
@ahumesky ahumesky added the team-Android Issues for Android team label Apr 15, 2022
@nkoroste
Copy link
Contributor

nkoroste commented May 3, 2022

Any updates on this?

@ckolli5 ckolli5 added the awaiting-review PR is awaiting review from an assigned reviewer label May 5, 2022
@bazel-io bazel-io closed this in 075c5a3 May 10, 2022
@ted-xie
Copy link
Contributor

ted-xie commented May 11, 2022

@nkoroste We've merged this upstream!

@nkoroste
Copy link
Contributor

Can we add it to 5.2 while we're at it? #15210

@ted-xie
Copy link
Contributor

ted-xie commented May 16, 2022

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label May 16, 2022
@ckolli5
Copy link

ckolli5 commented Jun 17, 2022

@bazel-io fork 5.3.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Jun 17, 2022
copybara-service bot pushed a commit that referenced this pull request Jun 22, 2022
Adding support for multiplex workers inside of `ResourceProcessorBusyBox` and moving it's worker implementation over to the generic work request handler.

These PRs need to land for this to work:
- #14424
- #14425
- #14427

For those not on rolling releases, the other required PRs that have already merged are:
- #14144
- #14145
- #14146

Closes #14428.

PiperOrigin-RevId: 456561596
Change-Id: I098d5a323ac6558ad0f5f8190e29f45a7a37b4d4
aranguyen pushed a commit to aranguyen/bazel that referenced this pull request Jun 27, 2022
Adding support for multiplex workers inside of `ResourceProcessorBusyBox` and moving it's worker implementation over to the generic work request handler.

These PRs need to land for this to work:
- bazelbuild#14424
- bazelbuild#14425
- bazelbuild#14427

For those not on rolling releases, the other required PRs that have already merged are:
- bazelbuild#14144
- bazelbuild#14145
- bazelbuild#14146

Closes bazelbuild#14428.

PiperOrigin-RevId: 456561596
Change-Id: I098d5a323ac6558ad0f5f8190e29f45a7a37b4d4
@ckolli5
Copy link

ckolli5 commented Jun 29, 2022

Hello @ted-xie, I cherry-picked these changes and raised a PR against release-5.3.0 branch. Presubmit checks are failing in that PR. Could you please cherry picked with appropriate commits and raise a PR against release-5.3.0? Thanks!

ted-xie pushed a commit to ted-xie/bazel that referenced this pull request Jul 6, 2022
Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

Closes bazelbuild#14427.

PiperOrigin-RevId: 447808701
ted-xie pushed a commit to ted-xie/bazel that referenced this pull request Jul 6, 2022
Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

Closes bazelbuild#14427.

PiperOrigin-RevId: 447808701
ted-xie pushed a commit to ted-xie/bazel that referenced this pull request Jul 6, 2022
Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

Closes bazelbuild#14427.

PiperOrigin-RevId: 447808701
ckolli5 pushed a commit that referenced this pull request Jul 6, 2022
* Support uses-permission merging in manifest merger

Adding support for conditionally merging `uses-permissions`.

#10628
#5411

Closes #13445.

RELNOTES: Enable merging permissions during Android manifest merging with the --merge_android_manifest_permissions flag.
PiperOrigin-RevId: 439613035

* Make ManifestMergerAction worker compatible

Calling `System#exit` kills the worker during the build. Passing the exception up to the worker should be enough for it to end up in the worker or local execution output.

Closes #14427.

PiperOrigin-RevId: 447808701

* Fix ManifestMergerAction test case on windows

`manifest.toString().replaceFirst("^/", "")` silently fails on windows machines causing `removePermissions` to write to the original test file. This pull request creates a new temp file that `removePermissions` can write the modified manifest to.

Pulling this change out of another PR so that it's easier to merge. Original PR here https://github.com/bazelbuild/bazel/pull/13445/files#r631575251

Closes #13760.

PiperOrigin-RevId: 438643774

Co-authored-by: Ben Lee <ben@ben.cm>
@ShreeM01 ShreeM01 removed the awaiting-review PR is awaiting review from an assigned reviewer label Sep 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-Android Issues for Android team team-Performance Issues for Performance teams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants