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

fix(android): app builds fail with uppercase module JARs on case-sensitive systems #11417

Merged
merged 3 commits into from Jan 9, 2020

Conversation

jquick-axway
Copy link
Contributor

JIRA:
https://jira.appcelerator.org/browse/TIMOB-27706

Note:
This is mostly an issue on Linux, but can be reproduced on macOS. Titanium's only module that has an uppercase letter is "ti.facebook", which contains a "Facebook.jar".

This is not an issue with the module id. It is an issue with module "name" set in its "manifest" file.

Test:
Following the test procedure in TIMOB-27706 and verify that the app no longer crashes when accessing Facebook in kitchensink-v2.

- TIMOB-27706: App built from case-sensitive volume will fail if referencing a native module JAR having uppercase letters.
  * This has been an issue for several years. Mostly an issue on Linux.
  * We were wrongly doing a toLowerCase() on the JAR file name.
@build
Copy link
Contributor

build commented Jan 8, 2020

Messages
📖

💾 Here's the generated SDK zipfile.

📖 ✊ The commits in this PR match our conventions! Feel free to Rebase and Merge this PR when ready.
📖

✅ All tests are passing
Nice one! All 6550 tests are passing.
(There are 700 skipped tests not included in that total)

Generated by 🚫 dangerJS against aca110d

Copy link
Contributor

@sgtcoolguy sgtcoolguy left a comment

Choose a reason for hiding this comment

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

fix looks good.

May want to explicitly add comment/note that when building modules we are generating the json file with a forced lowercase name from the manifest, but when we generate the module/JAR/project we retain the case from the manifest. (or something to link exactly why this change/fix was required and case sensitivity is important here, maybe even a pointer to the code in _buildModule.js where we're passing the name as-is and converting to lowercase - or even better yet, find a way to extract out a common function to enforce the proper name/case is used in both building and consumption of modules!).

@ssekhri
Copy link

ssekhri commented Jan 8, 2020

FR passed.
Verified on:
Mac OS: 10.15.1
SDK: 9.0.0.v20200107215936
Appc CLI: 7.1.2
JDK: 11.0.4
Node: 10.16.3
Studio: 6.0.0.201911251516
Device: Pixel3(v10.0) emulator

@ssekhri
Copy link

ssekhri commented Jan 8, 2020

Waiting for pending CR before merge

@jquick-axway
Copy link
Contributor Author

@sgtcoolguy, I'll change the code to not lowercase the JSON files too. The externalized JSON is a new thing in 9.0.0 so it's okay. Older built modules embedded the JSON within the JAR, which I got rid of because it was bloating the built APK (typically by a couple megs).

- This is a new JSON file introduced in 9.0.0 that older modules are not producing yet.
@jquick-axway
Copy link
Contributor Author

Updated PR to not lowercase binding JSON file.

Note that none of our released modules produce this JSON file yet. This is something we're introducing for 9.0.0 built modules. So, there's nothing to test yet (but I've tested it on my end already).

@ssekhri
Copy link

ssekhri commented Jan 8, 2020

FR passed with latest changes.
Verified on:
Mac OS: 10.15.1
SDK: 9.0.0.v20200108134222
Appc CLI: 7.1.2
JDK: 11.0.4
Node: 10.16.3
Studio: 6.0.0.201911251516
Device: Pixel3(v10.0) emulator

@sgtcoolguy sgtcoolguy merged commit 9bf5364 into tidev:master Jan 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants