Skip to content
This repository has been archived by the owner on May 9, 2022. It is now read-only.

TY-1786 build dart example #71

Merged
merged 18 commits into from
May 20, 2021
Merged

Conversation

Robert-Steiner
Copy link
Contributor

@Robert-Steiner Robert-Steiner commented May 11, 2021

Ticket: TY-1786

Changes:

  • added job for building the dart example
  • replaced cache action with upload\ download artifact (the cache step did not work for some unknown reason on macOS)
  • made sure that we call flutter build with same option as Blue does
  • updated flutter to 2.0.6

Android

https://github.com/xaynetwork/xayn_search/blob/27d2291f30446c15ff43d171867f889149b93a87/.github/workflows/xayn_search_master_pr.yml#L73-L79

iOS

https://github.com/xaynetwork/xayn_search/blob/27d2291f30446c15ff43d171867f889149b93a87/.github/workflows/xayn_search_master_pr.yml#L95-L98

  • updated xcode build settings
    • exculded build for armv7
    • disabled bitcode option for all pods
    • updated the minimum required iOS sdk version from 8 to 12.1
    • everything else was updated/added by flutter or xcode

Open questions

  • do we want to remove the flutter cache step? It doesn't save that much time, but it does take up a lot of space
  • do we also want to build for release? We don't expect any differences between release and debug build on our par codet, so debug should be enough

https://github.com/xaynetwork/xayn_search/blob/27d2291f30446c15ff43d171867f889149b93a87/xayn_search/build_scripts/build_ios_for_release.sh#L55

  • do we want to upgrade flutter to the same version Blue is using? (beta channel)

@Robert-Steiner Robert-Steiner marked this pull request as ready for review May 11, 2021 11:05
Copy link
Contributor

@janpetschexain janpetschexain left a comment

Choose a reason for hiding this comment

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

lgtm 👍

do we want to remove the flutter cache step? It doesn't save that much time, but it does take up a lot of space

i'd say yes, but that's more of a gut feeling. it seems we should save the cache space and might use it in another git action if we don't gain speedup from it.

do we also want to build for release?

there is probably no need for it, as it should only affect performance, but not logic. also with a debug build we get the occasional debug assertions.

do we want to upgrade flutter to the same version Blue is using? (beta channel)

if there is no compatibility issue, i'd say no as we should aim for stability in production releases. does blue rely on any beta features or could they use stable as well?

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from 572d349 to e032772 Compare May 12, 2021 11:57
@Robert-Steiner
Copy link
Contributor Author

if there is no compatibility issue, i'd say no as we should aim for stability in production releases. does blue rely on any beta features or could they use stable as well?

had a quick chat with frank and team blue uses beta because the null safe version of flutter_driver only exists there. But it will be soon out of beta so we can keep stable here.

@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from f3dc14c to 27a60d3 Compare May 12, 2021 15:33
@Robert-Steiner
Copy link
Contributor Author

Robert-Steiner commented May 17, 2021

Okay, I've added the xcode archive command and we can reproduce the bug the team encountered

 ld: could not reparse object file in bitcode bundle: 'Unknown attribute kind (304) (Producer: 'LLVM12.0.0-rust-1.52.1-stable' Reader: 'LLVM APPLE_1_1200.0.32.29_0')', using libLTO version 'LLVM version 12.0.0, (clang-1200.0.32.29)' for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from ed46515 to 38b3a54 Compare May 17, 2021 12:04
@Robert-Steiner
Copy link
Contributor Author

Robert-Steiner commented May 17, 2021

Okay, I've added the xcode archive command and we can reproduce the bug the team encountered

 ld: could not reparse object file in bitcode bundle: 'Unknown attribute kind (304) (Producer: 'LLVM12.0.0-rust-1.52.1-stable' Reader: 'LLVM APPLE_1_1200.0.32.29_0')', using libLTO version 'LLVM version 12.0.0, (clang-1200.0.32.29)' for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

has been fixed fixed in #75

The new linker error, seems to be spefic to the XCode version 12.4

ld: warning: ignoring file /Users/runner/work/xayn_ai/xayn_ai/bindings/dart/ios/libxayn_ai_ffi_c_x86_64-apple-ios.a, building for iOS-arm64 but attempting to link with file built for unknown-x86_64
ld: Invalid record for architecture arm64
clang: error: linker command failed with exit code 1 (use -v to see invocation)

The build does not fail with 12.5 (latest release but has not been rolled out yet) or 12.2 (the version that Team Blue uses)

I will change it to 12.2 to match Team Blue's CI settings.

Not sure if we should also change the XCode version in the ios build jobs to 12.4.

@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from e9ef165 to 652a728 Compare May 17, 2021 16:31
Copy link
Contributor

@acrrd acrrd left a comment

Choose a reason for hiding this comment

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

I think is good, I'm a bit worried that this file is getting too big. May we can think about splitting this in two (moving out the part needed for flutter build) or three (rust, flutter, flutter build)
and see if we can move the built libraries among the workflows using artifacts.

run: mv xayn-ai-ffi-c/ffi.h ${{ github.workspace }}/xayn-ai-ffi-c/ffi.h

- name: Download all artifacts
# Currently we can only download all artifacts. Using wildcards is not
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use build-ios-${{ matrix.target }}-${{ github.sha }} as the name of the artifact we should be able to name it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good Idea👍 I will change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -21,6 +21,6 @@
<key>CFBundleVersion</key>
<string>1.0</string>
<key>MinimumOSVersion</key>
<string>8.0</string>
<string>9.0</string>
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍 What I noticed is that team blue uses different versions across the project. In AppFrameworkInfo.plist they use 10 and in the xcode project they use 12.1
But in the flutter docs they say they should actually match https://flutter.dev/docs/deployment/ios#updating-the-apps-deployment-version
Shall we go with 12.1?

Copy link
Contributor

Choose a reason for hiding this comment

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

Point that out in their channel, I have no idea what the correct value should be.

@@ -0,0 +1,41 @@
# Uncomment this line to define a global platform for your project
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this now?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need it to set some custom options for our pods/dependencies. Before we just used the default settings that are generated by flutter when the file doesn't exist.

@@ -235,6 +302,7 @@
isa = XCBuildConfiguration;
buildSettings = {
ALWAYS_SEARCH_USER_PATHS = NO;
ARCHS = arm64;
Copy link
Contributor

Choose a reason for hiding this comment

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

Blue does not have this but has VALID_ARCHS = "arm64 arm64e" do you know the difference between the two options?
https://github.com/xaynetwork/xayn_search/blob/50ca127d7641613888ae62fa62f7a3cbf2fe48f4/xayn_search/ios/Runner.xcodeproj/project.pbxproj#L520

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Valid arch has been deprecated since Xcode 12.x

The Build Settings editor no longer includes the Valid Architectures build setting (VALID_ARCHS), and its use is discouraged. Instead, there is a new Excluded Architectures build setting (EXCLUDED_ARCHS). If a project includes VALID_ARCHS, the setting is displayed in the User-Defined section of the Build Settings editor. (15145028)

Xcode release notes

I replaced it with EXCLUDED_ARCHS
I will let team blue know about it

@@ -431,8 +444,8 @@ jobs:
RUSTFLAGS: ${{ env.OPT_TESTS_RUSTFLAGS }}
run: wasm-pack test --firefox --chrome --headless

build-xain-ai-lib:
name: build-xain-ai-lib
build-linux-lib:
Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering if we can reuse what is built during the test job.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cargo test does not produce the library but we can reuse the cache of the test job. Then only the library has to be created and not everything else. I will change it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
@acrrd
Copy link
Contributor

acrrd commented May 17, 2021

  • replaced cache action with upload\ download artifact (the cache step did not work for some unknown reason on macOS)

There was a bug that we solved by installing the gnu version of tar. But I thought that it was solved in the new version of the macos image.

do we also want to build for release? We don't expect any differences between release and debug build on our par codet, so debug should be enough

We don't run the example app, the libraries are just there to make flutter build ios happy but they are not really used so debug is fine.

For the rest I agree with what @janpetschexain already said.

@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from ff193f5 to 3253c5a Compare May 19, 2021 08:11
@Robert-Steiner
Copy link
Contributor Author

Robert-Steiner commented May 19, 2021

The build does not fail with 12.5 (latest release but has not been rolled out yet) or 12.2 (the version that Team Blue uses)

That statement was wrong. The Xcode version was not the reason it didn't work in the CI, but the fact that I compiled the static lib locally without the cargo flag CARGO_INCREMENTAL=0. I think it has something to do with TY-1793 why this error does not occur with CARGO_INCREMENTAL=1.

The root cause seems to be debuginfo. With debuginfo enabled the compiler creates bitcode that is incompatible with apples bitcode format. There is an open issue about the incompatibility.

We could try to build rust std with the llvm version that apple uses. There is already a repo rust-bitcode that we could use to do that but it requires rust nightly and I'm not sure we're causing other problems because of this.

For what is bitcode used for:

So why does Apple require bitcode uploads for the watchOS and tvOS? Well, by moving the uploads to a centralised Apple server it is possible for Apple to optimise the binaries between compilation with Xcode and delivery to the target device. It's also possible for developers to upload multiple variants and instead of packaging them into a single delivery (which would take up more space on the device). Finally, it also allows Apple to perform the code-signing of the application on the server side, without exposing any keys to the end developer.

There are even more advantages.

The good thing is that bitcode is only required for watchOS and tvOS apps (for now).

Flutter has added support for bitcode enabled apps but it seems to be disabled by default, however, that is not the case for the pods/dependencies of the app. So to make the xcodebuild command work with the build that contains debuginfo we just need to disable bitcode in build settings of the pods.

@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from 6e4ad9b to a071b95 Compare May 20, 2021 07:43
@Robert-Steiner Robert-Steiner force-pushed the TY-1786-build-dart-example branch from 407b586 to 3ec60be Compare May 20, 2021 09:10
@Robert-Steiner Robert-Steiner merged commit bb23d87 into master May 20, 2021
@Robert-Steiner Robert-Steiner deleted the TY-1786-build-dart-example branch May 20, 2021 17:27
acrrd pushed a commit that referenced this pull request Jun 28, 2021
add build dart example 

Co-authored-by: janpetschexain <58227040+janpetschexain@users.noreply.github.com>
acrrd pushed a commit that referenced this pull request Aug 24, 2021
add build dart example 

Co-authored-by: janpetschexain <58227040+janpetschexain@users.noreply.github.com>
acrrd pushed a commit that referenced this pull request Nov 4, 2021
add build dart example

Co-authored-by: janpetschexain <58227040+janpetschexain@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants