chore: adopt dart native build hooks#236
Conversation
|
This is awesome! But we probably need to fix lints and get the tests to pass :D |
|
Do we have any idea what is wrong with the desktop platforms? |
HamdaanAliQuatil
force-pushed
the
native-hooks
branch
from
69dc9ac to
41d9fa3
Compare
HamdaanAliQuatil
force-pushed
the
native-hooks
branch
2 times, most recently
from
dacb521 to
5a70dff
Compare
HamdaanAliQuatil
force-pushed
the
native-hooks
branch
from
a67960b to
aa44b69
Compare
| } | ||
|
|
||
| final targetOs = input.config.code.targetOS; | ||
| if (!_hostSupports(targetOs)) { |
There was a problem hiding this comment.
So we don't support building for Android?
There was a problem hiding this comment.
I had added a guard in the hook that restricted it to desktop targets while testing the VM path, and I failed to remove it afterward. I’ve removed that restriction now.
Android still passes in CI after that change. The Android log also shows native build-related activity, including native_toolchain_cmake being resolved and a rebuild after File modified during build.
| Abi.linuxArm64, | ||
| Abi.macosArm64, | ||
| Abi.windowsArm64, | ||
| }.contains(Abi.current()); |
There was a problem hiding this comment.
Abi.current() is the ABI of the Dart process running the hook.
I had used it as a host-architecture guard, but that was the wrong abstraction here since the actual target already comes from input.config.code. Ive removed it.
| packageRoot.resolve('src/webcrypto.c'), | ||
| packageRoot.resolve('src/webcrypto.h'), | ||
| packageRoot.resolve('src/symbols.generated.c'), | ||
| packageRoot.resolve('third_party/boringssl/sources.cmake'), |
There was a problem hiding this comment.
We have way more dependencies? What is this?
There was a problem hiding this comment.
Those are hook invalidation dependencies, not linker dependencies.
I was using a small hand-maintained list of obvious inputs, but that seems too ad hoc. I've changed it to collect actual native build inputs from src/ and third_party/boringssl/ instead.
|
|
||
| concurrency: | ||
| group: ${{ github.workflow }}-${{ github.ref }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
where does this come from and how is it related?
There was a problem hiding this comment.
Why do we need to change CI in this PR?
If we need to change CI, can we do it in an independent PR that discusses why are making those changes.
| - run: flutter test --coverage | ||
| - run: flutter test --platform chrome | ||
| - run: flutter test --platform chrome --wasm | ||
| - run: dart test -p chrome -c dart2wasm |
There was a problem hiding this comment.
This very cool, now we can test with dart test too.
| - name: Run integration_test with chromedriver | ||
| working-directory: ./example | ||
| run: | | ||
| xvfb-run ../tool/with-chromedriver.sh flutter drive \ |
There was a problem hiding this comment.
I'm guessing this stopped working, but that should land in a different PR.
3 participants
Closes #192