search

tidev / hyperloop.next

Hyperloop Next version (we forgot the version number at this point)
Other
8 stars 5 forks source link

Refactor iOS, add support for dotted enums, run native tests, improve caching #271

Closed sgtcoolguy closed 3 years ago

sgtcoolguy commented 6 years ago

Follow-on to #270

This does a lot:

To investigate:

Future work:

hansemannn commented 6 years ago

@sgtcoolguy Can you resolve the merge conflicts? I'd love to give it an early shot already.

hansemannn commented 6 years ago

+1 for not running pod install on every build. We could likely use the Podfile.lock + some fs'ing to check if the Pods are installed already. In addition, I just opened https://github.com/CocoaPods/CocoaPods/issues/7628 to see if the CocoaPods team may have something in place already.

sgtcoolguy commented 6 years ago

@hansemannn I should have been more specific. We do manage to avoid unnecessarily re-running pod install every time. We check for a Podfile.lock and our own custom cache file marker in the build directory. (it looks like what they do here is more correct and would remove the need for our own custom file? https://github.com/square/cocoapods-check/blob/master/lib/pod/command/check.rb)

The issue is that I don't try to determine if we can skip running xcodebuild against the pods to generate the built frameworks. I assume I can do a pretty simple check to verify if the output directory exists and if some custom cache file we place there matches (based on the Podfile.lock sha as we do to avoid the pod install above).

sgtcoolguy commented 6 years ago

OK, just pushed an update that uses the same concept of writing a cache file with a sha to the build dir to avoid re-running cocoa pods builds unnecessarily.

I think this should avoid re-running xcodebuild for cocoa pods when we don't need to (though I think I have to fix it to detect if you run ti clean so it doesn't see a good cache file and skip even though the output directory isn't there any more).

hansemannn commented 6 years ago

I will try it out tomorrow. And I've updated the title to indicate that this PR is not a simple new feature :-). And I just bumped the version to 3.2.0, can you pull from master? Testing the 3.2.0 artifact then.

hansemannn commented 6 years ago

Super low prio feature request: Instead of printing the actual build command of each Pod to the console, it'd look super rad if we had something like:

[ ✅] Building GLCalendarView ... Done!
[ 🔄] Building JBChartView ...

I think we have some kind of console loader so far, so it could be used across the build.

Just a thought -- maybe for Hyperloop X. :-)

hansemannn commented 6 years ago

First PR testing in progress. Clean build took ~ 3mins, second build crashed:

[INFO]  Starting Hyperloop assembly
[INFO]  Finished Hyperloop assembly in 1.1 seconds
[ERROR] An error occurred during build after 24s 210ms
[ERROR] /Users/hknoechel/.hyperloop/metabase-CoreGraphics-08bcb081fccc5fa454cd6c7739df421a.json: Unexpected token  in JSON at position 0

Character 0 is { and looks like valid JSON. Also, it could likely be minified to save space. Oh, and it looks like all other framework stubs are empty (0 bytes).

sgtcoolguy commented 6 years ago

Oopsie, I had a bad error in logic with Promises here for metabase generation (my understanding of Promises wasn't right). In any case I just pushed a fix so we don't re-generate the metabase for a framework when we have a cached copy. That sped things up a bit for me (duh!).

For me locally with hyperloop-examples:

If I do a full clean build with cocoapods not installed:

rm -rf Pods/
rm -rf Podfile.lock
rm -rf ~/.hyperloop
ti clean
ti build -p ios

That build takes ~1m 30s.

If I take cocoapods install out of the equation:

rm -rf ~/.hyperloop
ti clean
ti build -p ios

That build takes ~1m 5s.

Then I change a couple lines in a controller and rebuild and it takes ~15s. Added require of a new framework to a controller (Twitter) and rebuild without clean: 17s.

Note that this is what the CLI is reporting to me as the time to build, not the actual time for the app to launch/etc.

sgtcoolguy commented 6 years ago

For reference I rolled back hyperloop-examples to hyperloop 3.0.3/SDK 7.1.0GA.

Clean build took 1m 8s. Rebuild with no changes: 13s. Turned transpile on and rebuilt: 28s. Added require of new framework ('Twitter') and rebuilt without clean: 24s.

So to sum up: The changes here do make for a slower first build - which makes sense. The old method generated a single unified metabase based on your exact usage, so it's doing less work. The new method will generate metabases for each framework separately and generates more wrappers, so it's doing more work.

But the new code is able to re-used cached metabases across projects (for system frameworks) and is able to incrementally add metabases as you reference new frameworks in your code.

The gain here isn't as large as I was hoping, but over development time shaving off 7s per build whenever you add/remove a reference to a new Framework (or possibly even just new types in frameworks) seems useful.

I do feel like there's still plenty of opportunity to tune this as well. I don't think we're generating the wrappers in parallel or any of that...

Oh yeah, and we're still parsing and traversing the AST of each file twice in hyperloop. Yuck, fixing that ought to help...

hansemannn commented 6 years ago

Sweeeet! Will test the updates

hansemannn commented 6 years ago

Build Time

1st run, no global metabase cached: 3m 26s 31ms Rebuild with no changes: 49s 801ms

Another clean build, global metabase cached: 3m 23s 802ms

Very time consuming tasks are:

-> Can't we skip the check for each file if we know nothing changed at all? Note that this is only seen for CocoaPods deps.

I think these are general SDK pipeline issues though.

Note: I was using both the ES6 PR (https://github.com/appcelerator/hyperloop-examples/pull/76) and master, nearly same results.

General Bugs

Summary

Something is weird here. There is a bin/ directory created in the project root, containing something that is looking like a copy of the project. Also, the .gradle directory seems new, which can be okay.

sgtcoolguy commented 6 years ago

So some findings:

hansemannn commented 6 years ago

@sgtcoolguy Jenkins failed and I do not have permissions to retrigger it.

hansemannn commented 6 years ago

@sgtcoolguy Are the issues supposed to work so far or do I need to flush the global metabase? It still crashes in the above scenarios, but I don't want to test prematurely.

sgtcoolguy commented 6 years ago

@hansemannn The custom class bug hasn't been fixed yet, but the Tableview/Collection View, Interface Builder UI should work now. You'll need to flush ~/.hyperloop first as the metabases now contain a new top-level key holding type extensions.

I'd like to fix it more properly, as I'm certain there are still cases it could fall down. Basically if a type is extended in another framework we record the additional class definition under it's name in the extensions part of the metabase. I haven't handled if there are multiple extensions of the same type in the same framework (where the base type is in another one). I need to modify the metabase generator to merge those together rather than clobber and last one wins.

Then when we generate the stubs we have the full unified metabase from all the used frameworks. If a class has extensions in that unified metabase we merge those properties/methods into the base type before we write the stub - in effect achieving what we did before.

hansemannn commented 6 years ago

Will try now! Btw, should we move the global Android metabase to ~/.hyperloop as well? :-)

sgtcoolguy commented 6 years ago

@hansemannn when I ran a clean and rebuilt the custom class issue was fixed for me. So I think all the examples work now, except maybe Localytics (though maybe that's broken outside this PR?)

The Android side still generates a single unified metabase on-disk. It doesn't make a lot of sense to move the metabase to ~/.hyperloop yet until it also does per-library metabases - then the JDK/Android SDK metabase could be re-used cross-project - and likely we could generate shared metabases for aars/jars based on some hash of their contents or something - so if multiple projects used the same libraries they'd re-used the same metabase.

In that case, I may also want to consider the same for the cocoapods frameworks on iOS. If it's the same pod name/version/sdk version we should be able to re-use the metabase cross-project...

hansemannn commented 6 years ago

@sgtcoolguy Coming back to this PR. Our 3.1.0 release is done, but I would personally take this one as well depending on your approval. We could release it over the upcoming week, include it in 7.2.0 and receive feedback early. If there is an issue between now and the 7.2.0 release (not before end of June probaly), we could still get the feedback from early testers until then. Does that sound like a plan?

hansemannn commented 6 years ago

@sgtcoolguy Please pull from master again. I would enable branch protection but do not have the permission anymore.

EDIT: Now it has merge conflicts. Resolved.

hansemannn commented 6 years ago

@sgtcoolguy There seem to be a character-index issue with dotted enums. This what I am trying to do (via https://github.com/appcelerator/hyperloop-examples/pull/77):

import {  UIControlEvents } from 'UIKit';
alert(UIControlEvents.TouchUpInside); // Does not work
alert(UIControlEvents.ouchUpInside); // Works :-D

This is generated:

/**
 * HYPERLOOP GENERATED - DO NOT MODIFY
 *
 * This source code is Copyright (c) 2018 by Appcelerator, Inc.
 * All Rights Reserved.  This code contains patents and/or patents pending.
 */

var UIControlEvents = {};

// value
Object.defineProperties(UIControlEvents, {

    llEditingEvents: {
        value: 983040,
        enumerable: false,
        writable: false
    },

    llEvents: {
        value: 4294967295,
        enumerable: false,
        writable: false
    },

    llTouchEvents: {
        value: 4095,
        enumerable: false,
        writable: false
    },

    pplicationReserved: {
        value: 251658240,
        enumerable: false,
        writable: false
    },

    ditingChanged: {
        value: 131072,
        enumerable: false,
        writable: false
    },

    ditingDidBegin: {
        value: 65536,
        enumerable: false,
        writable: false
    },

    ditingDidEnd: {
        value: 262144,
        enumerable: false,
        writable: false
    },

    ditingDidEndOnExit: {
        value: 524288,
        enumerable: false,
        writable: false
    },

    rimaryActionTriggered: {
        value: 8192,
        enumerable: false,
        writable: false
    },

    ystemReserved: {
        value: 4026531840,
        enumerable: false,
        writable: false
    },

    ouchCancel: {
        value: 256,
        enumerable: false,
        writable: false
    },

    ouchDown: {
        value: 1,
        enumerable: false,
        writable: false
    },

    ouchDownRepeat: {
        value: 2,
        enumerable: false,
        writable: false
    },

    ouchDragEnter: {
        value: 16,
        enumerable: false,
        writable: false
    },

    ouchDragExit: {
        value: 32,
        enumerable: false,
        writable: false
    },

    ouchDragInside: {
        value: 4,
        enumerable: false,
        writable: false
    },

    ouchDragOutside: {
        value: 8,
        enumerable: false,
        writable: false
    },

    ouchUpInside: {
        value: 64,
        enumerable: false,
        writable: false
    },

    ouchUpOutside: {
        value: 128,
        enumerable: false,
        writable: false
    },

    alueChanged: {
        value: 4096,
        enumerable: false,
        writable: false
    }

});

module.exports = UIControlEvents;

It seems like the first character is always cutted out.

hansemannn commented 6 years ago

More feedback from the community:

sgtcoolguy commented 3 years ago

unfortunately this PR is super outdated, killing it off