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

🏗️ Prototype: Android + Cmake #4301

Closed
wants to merge 25 commits into from
Closed

Conversation

strseb
Copy link
Collaborator

@strseb strseb commented Aug 30, 2022

Description

Switch build pipeline from:

Script.->Qmake->make->qmlimportscanner->gradle

We will build the client with cmake. Cmake will be invoked by gradle.
This allow's us to build the client with one command (theoreticly) gradlew assemble, and gives us the ability to define cmake/ndk versions as a classic gradle dependency.

Handling can stay the same as the package.sh now will just set gradle up, instead of qmake :)

@strseb strseb changed the title 🏗️ Prototype: Android -> Gradle -> Cmake 🏗️ Prototype: Android + Cmake Aug 31, 2022
@strseb strseb force-pushed the basti/prototype/sadCmake branch from 27e803b to cd9ca64 Compare August 31, 2022 11:14
@strseb strseb force-pushed the basti/prototype/sadCmake branch from cd9ca64 to 8c016f0 Compare August 31, 2022 11:22
@strseb strseb force-pushed the basti/prototype/sadCmake branch from fe625dd to cb1598c Compare August 31, 2022 12:26
@strseb strseb mentioned this pull request Sep 14, 2022
5 tasks
@strseb strseb requested a review from oskirby September 22, 2022 15:13
Copy link
Collaborator

@bakulf bakulf left a comment

Choose a reason for hiding this comment

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

Can you remove qmake/platforms/android.pri and add something like this:
https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/src/src.pro#L44

CMakeLists.txt Outdated
# Otherwise the NDK Toolchain wont find qt
set(CMAKE_FIND_ROOT_PATH_MODE_PACKAGE NEVER)

set(QT_HOST_PATH "~/Code/Qt/6.2.4/macos")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove this and use and env var?

@@ -7,5 +7,5 @@ object Config {
const val buildToolsVersion = "30.0.3"
const val minSdkVersion = 24
const val targetSdkVersion = 30
const val ndkVersion = "23.1.7779620"
const val ndkVersion = "24.0.8215888"
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we need to update the documentation in the README file or elsewhere?

@@ -0,0 +1,226 @@
import org.gradle.api.Plugin
Copy link
Collaborator

Choose a reason for hiding this comment

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

headers

@@ -0,0 +1,43 @@
import org.gradle.api.Plugin
Copy link
Collaborator

Choose a reason for hiding this comment

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

headers

android/buildSrc/src/main/kotlin/QtConfig.kt Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2022

Codecov Report

Base: 56.36% // Head: 16.40% // Decreases project coverage by -39.96% ⚠️

Coverage data is based on head (68309d2) compared to base (88dbef3).
Patch has no changes to coverable lines.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4301       +/-   ##
===========================================
- Coverage   56.36%   16.40%   -39.97%     
===========================================
  Files          12       71       +59     
  Lines         605     3719     +3114     
  Branches      288     1789     +1501     
===========================================
+ Hits          341      610      +269     
- Misses        264     2794     +2530     
- Partials        0      315      +315     
Flag Coverage Δ
lottie_tests 56.36% <ø> (ø)
qml_tests 7.51% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/featureslistcallback.h 0.00% <ø> (ø)
lottie/lib/lottieprivate.cpp 13.63% <0.00%> (ø)
lottie/lib/lottieprivatewindow.cpp 76.81% <0.00%> (ø)
src/urlopener.cpp 0.00% <0.00%> (ø)
src/filterproxymodel.cpp 54.71% <0.00%> (ø)
src/logger.cpp 22.00% <0.00%> (ø)
src/qmlengineholder.h 100.00% <0.00%> (ø)
tests/qml/main.cpp 100.00% <0.00%> (ø)
src/mozillavpn.h 80.00% <0.00%> (ø)
src/networkmanager.cpp 9.80% <0.00%> (ø)
... and 52 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@strseb strseb force-pushed the basti/prototype/sadCmake branch from 79b6183 to 522ff0b Compare September 27, 2022 22:30
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
android/buildSrc/src/main/kotlin/QtConfig.kt Outdated Show resolved Hide resolved
@strseb strseb force-pushed the basti/prototype/sadCmake branch 3 times, most recently from eb75013 to b029cde Compare October 27, 2022 11:49
@strseb strseb force-pushed the basti/prototype/sadCmake branch from b029cde to 967339e Compare October 27, 2022 12:17
val importScannerList = arrayOf(
File("$host/libexec/qmlimportscanner"),
File("$host/bin/qmlimportscanner"),
File("$host/bin/qmlimportscanner.exe"),

Choose a reason for hiding this comment

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

🚫 [ktlint] reported by reviewdog 🐶
Unnecessary trailing comma before ")"

File("$host/bin/qmlimportscanner"),
File("$host/bin/qmlimportscanner.exe"),
)
val importScanner = importScannerList.firstOrNull{it.exists()}

Choose a reason for hiding this comment

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

🚫 [ktlint] reported by reviewdog 🐶
Missing spacing around "{"

File("$host/bin/qmlimportscanner"),
File("$host/bin/qmlimportscanner.exe"),
)
val importScanner = importScannerList.firstOrNull{it.exists()}

Choose a reason for hiding this comment

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

🚫 [ktlint] reported by reviewdog 🐶
Missing spacing before "}"

)
val importScanner = importScannerList.firstOrNull{it.exists()}
?: {
error("Did not found qmlimportscanner in ${importScannerList}")

Choose a reason for hiding this comment

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

🚫 [ktlint] reported by reviewdog 🐶
Redundant curly braces

@strseb strseb force-pushed the basti/prototype/sadCmake branch from b3891eb to 68309d2 Compare October 31, 2022 13:49
@strseb strseb mentioned this pull request Oct 31, 2022
@strseb strseb closed this Nov 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants