Better error message when ANDROID_SDK isn't set #2

bertmaher · 2016-03-26T05:43:07Z

We look at this environment variable to find zipalign, but if it's not set we just die with a KeyError. It'd be better to either (a) give a helpful error, or (b) just don't zipalign. (a) is probably better.

Summary: D6793418 exposed two dormant issues with resource XML file serialization. 1) XML files with strings originally in UTF16 would get rewritten in UTF8. That resulted in a smaller file size, and tripped an assert when no strings should have been changed. 2) The serialization code here doesn't behave identical to aapt with respect to duplicate strings. For some reason these differences only surfaced in messenger, not in fb4a. In general, removing failing asserts is a pretty crappy way to "fix" things, though in this case the differences I've noticed for #2 appear to only be cosmetic. Reviewed By: arnaudvenet Differential Revision: D7078031 fbshipit-source-id: 2f037904fab5b4434b4c3218387f85ca51b85350

Summary: D6793418 exposed two dormant issues with resource XML file serialization. 1) XML files with strings originally in UTF16 would get rewritten in UTF8. That resulted in a smaller file size, and tripped an assert when no strings should have been changed. 2) The serialization code here doesn't behave identical to aapt with respect to duplicate strings. For some reason these differences only surfaced in messenger, not in fb4a. In general, removing failing asserts is a pretty crappy way to "fix" things, though in this case the differences I've noticed for facebook#2 appear to only be cosmetic. Reviewed By: arnaudvenet Differential Revision: D7078031 fbshipit-source-id: 2f037904fab5b4434b4c3218387f85ca51b85350

Summary: Pull Request resolved: #427 Reviewed By: Feng23 Differential Revision: D17909600 Pulled By: justinjhendrick fbshipit-source-id: 6fb606871d9356a2d0982110af03fbd985abc692

Summary: Report: fbandroid/native/redex/libresource/VectorImpl.cpp:507:22: runtime error: null pointer passed as argument 2, which is declared to never be null fbcode/third-party-buck/platform007/build/glibc/include/string.h:43:28: note: nonnull attribute specified here Stack trace: ``` #0 android::VectorImpl::_do_copy (this=<optimized out>, dest=0x7fffd140c818, from=0x0, num=0) at fbandroid/native/redex/libresource/VectorImpl.cpp:507 #1 android::VectorImpl::setCapacity (this=0x7fffffff5ac0, new_capacity=25420576) at fbandroid/native/redex/libresource/VectorImpl.cpp:341 #2 0x0000000001f34f4a in android::Vector<char>::setCapacity (this=<optimized out>, size=17592186039131) at buck-out/gen/fbandroid/native/redex/libresource#header-mode-symlink-tree-with-header-map,headers/utils/Vector.h:85 #3 android::Vector<char>::reserve (this=<optimized out>, n=17592186039131) at buck-out/gen/fbandroid/native/redex/libresource#header-mode-symlink-tree-with-header-map,headers/utils/Vector.h:196 #4 android::ResTable::serialize (this=0x7fffffff66b0, cVec=..., resTableIndex=<optimized out>) at fbandroid/native/redex/libresource/ResourceTypes.cpp:4006 #5 0x0000000001e049eb in ResourcesArscFile::serialize (this=0x7fffffff66b0) at fbandroid/native/redex/libredex/RedexResources.cpp:1262 #6 0x00000000014ad94d in OptimizeResourcesPass::run_pass (this=0x24d25e0 <_ZL6s_pass>, stores=..., conf=..., mgr=...) at fbandroid/native/redex/facebook/opt/optimize_resources/OptimizeResources.cpp:630 #7 0x0000000001c6b2a9 in PassManager::run_passes (this=0x7fffffffb660, stores=..., conf=...) at fbandroid/native/redex/libredex/PassManager.cpp:258 #8 0x0000000000d2bb3a in main (argc=-25816, argv=0x7fffffffaf48) at fbandroid/native/redex/tools/redex-all/main.cpp:1122 ``` Reviewed By: int3 Differential Revision: D17889740 fbshipit-source-id: 360c1e7bd341a09f3537b9115cd2967d98b0c36c

Summary: I have been rebasing this each day since Thurs 12/10 and running locally to note which diffs would have been blocked: https://docs.google.com/spreadsheets/d/1qiFMrs149mL2q6qXeG0eAPfUd2rMLR5DDl1cCK5wbz8/edit?usp=sharing The main culprits include: 1) Using MobileConfig to gate things that happen to exist in the primary dex. 2) Bad code mods that are sprinkling Guava use cases everywhere. For #1, I think once existing packages like crash reporting get split up, this will become less of a problem over time. Or, once existing experiments in crash reporting infra are converted over to `FbColdStartExperiments` infra (a good thing to use from primary dex) new experiments would have good examples to copy. For #2, I've talked with the authors of the code mod and they're gonna use `//fbandroid/java/com/facebook/common/preconditions:preconditions` instead which isn't problematic. That said, the samples of the diffs that would valid validation is likely not going to be representative (end of year, people on PTO, etc). Though this is the best info I have right now. Also note, before landing I'll put out an Android FYI post and point people to the documentation. Here is the draft post: P155848149 Reviewed By: minjang Differential Revision: D25606766 fbshipit-source-id: e8089eb4159cbe9c85d05db9624797fbcafff789

Summary: This diff introduces two main changes: ### Update Graph interface Currently, `predecessors` and `successors` in Graph trait returns `&[Self::EdgeId]`, this would be fairly efficient for Graph implementations that can return a slice. But I realized that in many cases, the Graph implementations may only be able to return an iterator (e.g., using petgraph). To make the interface more flexible, I decide to update it. There are three options for the new return type: ``` // 1. A iterator trait object (lazily evaluated, dynamic dispatch). fn predecessors(&self, n: Self::NodeId) -> Box<dyn Iterator<Item = Self::EdgeId> + '_>; // 2. Vector (heap alloc). fn predecessors(&self, n: Self::NodeId) -> Vec<Self::EdgeId>; // 3. SmallVec is similar to LLVM SmallVector, which can hold data in stack if its size is smaller than S. fn predecessors(&self, n: Self::NodeId) -> SmallVec<[Self::EdgeId; S]>; ``` With some benchmarking using different numbers of successors (etc.,1, 2, 4, 6, 8, 10), it turns out that the option #1 and #2 has similar performance, while option #3 is always slightly better than the other two. So I decided to adopt it. This will introduce a new dependency to our library but it's small. (later I can put this kind of benchmarking code in a separate bench folder.) ### Add a new trait for getting successor nodes In WPO, we need an interface to query the successor nodes of a given node. Previously I used a closure for it (while the C++ version uses std::function). After discussing with yuxuanchen1997, we think it's better to make this a separate trait to achieve this, so we add `SuccessorNodes` (The type that implements Graph trait can also implement Fn trait, but it's not recommended in Rust). In most cases, the same type can implement both. But they can also be separate types. In addition, this diff adds a naive graph implementation for testing. In this diff we only use it to test the `get_succ_nodes` interface. Reviewed By: yuxuanchen1997 Differential Revision: D38846203 fbshipit-source-id: 7951b83a7df6817b3796c1a4e40d85e08b4c2c12

Summary: From what I can tell, the responsibilities of the python unpacker/repacker and redex-all binary are the following: 1) python unpacking logic creates an apk directory and a separate dex directory. 2) python layer also moves each dex file into a sub folder of the dex directory (i.e. classes.dex -> dex0/..., classes2.dex- > dex1/...) 3) redex-all binary outputs optimized dex files to the dex directory, which eventually will be packed back up along with the files in the apk directory. For the use case of something like informational-only passes, or a config which is only meant to do dead resource elimination (like the one supplied) we may want an option to ensure that dex files are not changed unintentionally. It is not immediately clear to me if round tripping to our IR and back will always produce the same code, not ever change debug info, etc etc, so I am adding an option to this. This change effectively turns step #2 above into a copy, tells redex-all binary to not write files, and asserts the file sizes have not been changed. Reviewed By: agampe Differential Revision: D47166165 fbshipit-source-id: 18cacd7e735da8d542d3efd7872677a348aed0d1

…o redex-stable"" Summary: Original commit changeset: 6131d6892539 Original Phabricator Diff: D54648671 A summary of issues: 1) Build failures on mac: https://fb.workplace.com/groups/buck2users/permalink/3610890652500623/, https://fb.workplace.com/groups/redex.feedback/posts/7155760447827011 2) Sporadic failures observed at land time and continuous builds via our own reporting: https://fburl.com/scuba/android_redex_failures/k9d8kiqi 3) Perhaps because of #1, I cannot land a speculative change for #2, see D54707997 (https://www.internalfb.com/sandcastle/workflow/4228880050103139544/artifact/actionlog.4228880050147425325.stderr.1?selectedLines=37320-37320-98-98) Reviewed By: beicy Differential Revision: D54714856 fbshipit-source-id: 4690b26427ca9bef8a4cbabc313b44fd2445c4fa

Summary: Implemented checking for s-expression patterns that can be a string or a number, and added test cases which check these. Additionally, added validation of types and error checking. Reviewed By: wsanville Differential Revision: D58156222 fbshipit-source-id: 553574f1baf63ea41ba046b6a55dafc7dbcff2ce

bertmaher closed this as completed Mar 30, 2016

Phil-Coulson mentioned this issue May 21, 2016

JNI DETECTED ERROR IN APPLICATION: JNI NewGlobalRef called with pending exception #123

Closed

colinstonema mentioned this issue Jul 10, 2017

Segmentation fault when run redex-all #231

Closed

deephole mentioned this issue Sep 1, 2017

Redex crashes with specific input during regalloc pass with compiler optimisations disabled #256

Closed

liush55 mentioned this issue Apr 19, 2018

redex crash when Running SingleImplPass with default.config #316

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better error message when ANDROID_SDK isn't set #2

Better error message when ANDROID_SDK isn't set #2

bertmaher commented Mar 26, 2016

Better error message when ANDROID_SDK isn't set #2

Better error message when ANDROID_SDK isn't set #2

Comments

bertmaher commented Mar 26, 2016