chore(android): update V8 to 7.3.492.26 #10817
Conversation
|
|
I assume this breaks native modules (i.e. we need to hold for 9.0 and bump the module api version)? Or did you manage to retain our patches to v8 that retained the removed APIs? Given that Android crashes on startup in our test suite, I'll bet that this means we need to re-build our modules, bump the api version and target this against "next" branch. |
|
@sgtcoolguy Currently working on that, should be able to get backwards compatibility. |
|
@sgtcoolguy Fixed native module compatibility, looks like Google Play Services needs updating on the emulator for the |
|
@garymathews this still crashes the android emulator - presumably backwards native module compat is still broken? |
|
Gary and I spoke about this on a back channel chat. Before merging I'd like to see the generation of the snapshot header file automated. Long-term I think that means mksnapshot binaries become packaged with the V8 libraries in the zip file on the v8_titanium side; and on this side, our build scripts to use the mksnapshot binaries to generate the header when the v8 library is updated and then hooked into our builds. That does present an issue if we're not generating a windows compatible mksnapshot binary and devs try to build the SDK on Windows (like Yordan does). |
garymathews
force-pushed
the
TIMOB-26909
branch
3 times, most recently
from
a6d267c
to
7886042
Compare
garymathews
force-pushed
the
TIMOB-26909
branch
from
365b722
to
04186e5
Compare
|
Made the necessary changes to V8 so that:
Once 7.3-lkgr has built, we can re-build this PR. |
| }; | ||
| return ant.build(ANDROID_BUILD_XML, [ 'full.build' ], properties); | ||
| // Clean build and download V8 | ||
| await this.clean().catch(error => console.error(error)); |
There was a problem hiding this comment.
really don't need the catch chained here, it'll bubble up the Error if there is one. adding a catch will actually just log it and move on.
|
|
||
| // Generate snapshots | ||
| const snapshot = require('./snapshot'); | ||
| await snapshot.build().catch(error => console.error(error)); |
There was a problem hiding this comment.
same here, no need for a catch really.
| ]; | ||
|
|
||
| // Set correct permissions for 'mksnapshot' | ||
| fs.chmodSync(MKSNAPSHOT_PATH, 0o755); |
There was a problem hiding this comment.
the method is async, so you could do await fs.chmod(MKSNAPSHOT_PATH, 0o755);
| return new Promise((resolve, reject) => { | ||
|
|
||
| // Snapshot already exists, skip... | ||
| if (fs.existsSync(BLOB_PATH)) { |
There was a problem hiding this comment.
this could be moved out of the Promise and use await fs.exists(BLOB_PATH)
| const blobs = {}; | ||
|
|
||
| // Load snapshots for each architecture | ||
| for (const target of TARGETS) { |
There was a problem hiding this comment.
again this method is async, so you can use the async fs methods.
Additionally, you could use probably refactor this very slightly to read in all the blobs in parallel by using Promise.all(TARGETS.map());
| } | ||
| } | ||
|
|
||
| return new Promise(async (resolve, reject) => { |
There was a problem hiding this comment.
I'm confused by the async here...
Generally I try to make use of util.promisify to handle older callback style APIs so I can avoid the hand-written Promise instances. so in this case you could do:
const promisify = require('util').promisify; // at the top of the file
const renderFile = promisify(require('ejs').renderFile);
// ...
const output = await renderFile(path.join(__dirname, 'V8Snapshots.h.ejs'), blobs, {});
return fs.writeFile(path.join(ANDROID_DIR, 'runtime', 'v8', 'src', 'native', 'V8Snapshots.h'), output);| * @returns {Promise<void>} | ||
| */ | ||
| async function build() { | ||
| return new Promise(async (resolve, reject) => { |
There was a problem hiding this comment.
again, if the method is async, you shouldn't need to hand write a Promise instance:
// Only macOS is supports creating snapshots
if (os.platform() !== 'darwin') {
return;
}
// generate snapshots in parallel
await Promise.all(TARGETS.map(t => generateBlob(t)));
return generateHeader();
7.3.492.26TIMOB-26957
TIMOB-26909