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

x/mobile/cmd/gomobile: ordering of manifest attributes may produce invalid manifest #13036

Closed
dskinner opened this issue Oct 23, 2015 · 6 comments

Comments

@dskinner
Copy link
Member

This subtle difference is not picked up in the tests since aapt will still successfully match the parsed xmltree against its own encoding of the same xml. The exact reason for the fault is yet unknown but can be worked around be reordering attributes in an apps manifest.

@dskinner
Copy link
Member Author

My initial reaction is to disable sorting of test results. Repeated encoding of the same manifest with the aapt tool appears deterministic, so sorting results may be obscuring a more fundamental issue.

go test -run BinaryXML -dump
unzip junk.sdk.apk
mv AndroidManifest.xml a.xml
go test -run BinaryXML -dump
unzip junk.sdk.apk
mv AndroidManifest.xml b.xml
diff a.xml b.xml
# no output

@hyangah hyangah added this to the Unreleased milestone Oct 23, 2015
@hyangah
Copy link
Contributor

hyangah commented Oct 23, 2015

cc @crawshaw

@dskinner
Copy link
Member Author

I've started working to resolve. Will also provide real testing down to byte-to-byte level, testing of any number of manifests, and have it setup so results can be tested against any future version of the sdk build-tools.

@dskinner
Copy link
Member Author

I forgot to mention localized testing down to the chunk header as well so any future errors can be more easily identified.

@dskinner
Copy link
Member Author

Here's a more formal proposal for how I plan to fix this (most of this is already working, the following being a header in the source):

// Binary resource structs support unmarshalling the binary output of aapt.
// Implementations of marshalling for each struct must produce the exact input
// sent to unmarshalling. This allows tests to validate each struct representation
// of the binary format as follows:
//
//  * unmarshal the output of aapt
//  * marshal the struct representation
//  * perform byte-to-byte comparison with aapt output per chunk header and body
//
// This process should strive to make structs idiomatic to make parsing xml text
// into structs trivial.
//
// Once the struct representation is validated, tests for parsing xml text
// into structs can become self-referential as the following holds true:
//
//  * the unmarshalled input of aapt output is the only valid target
//  * the unmarshalled input of xml text may be compared to the unmarshalled
//    input of aapt output to identify errors, e.g. text-trims, wrong flags, etc
//
// This provides validation, byte-for-byte, for producing binary xml resources.
//
// It should be made clear that unmarshalling binary resources is currently only
// in scope for proving that the BinaryMarshaler works correctly. Any other use
// is currently out of scope.

@dskinner
Copy link
Member Author

dskinner commented Mar 5, 2016

this has been addressed with #13109

@dskinner dskinner closed this as completed Mar 5, 2016
@golang golang locked and limited conversation to collaborators Mar 13, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants