Skip to content
This repository has been archived by the owner on Mar 26, 2020. It is now read-only.

Upgrade example build #339

Merged
merged 3 commits into from
Jan 4, 2018
Merged

Upgrade example build #339

merged 3 commits into from
Jan 4, 2018

Conversation

sschuberth
Copy link
Contributor

Please have a look at the individual commit messages for the details.

This is a follow-up to PR #329.

@sschuberth
Copy link
Contributor Author

/cc @xianwen

@artwyman
Copy link
Contributor

Can you explain where/why the null ndkDir would occur? I don't fully understand what the outcome would be if it's null, and thus the NDK build ends up being disabled. If that happened, it would invalidate the usefulness of a passing build as a regression test.

I'll leave @xianwen to do the final test & merge.

FYI, we've switched to mostly doing merges via Squash, so a PR summary is useful for forming the final commit message.

Also FYI since the CLA-bot is broken, I've confirmed that @sschuberth has signed the CLA.

@sschuberth
Copy link
Contributor Author

Can you explain where/why the null ndkDir would occur?

Simply if the NDK is not installed (but only the SDK). Granted, the example won't build in this case, but one might still want to be able to inspect the project e.g. by running ./gradlew tasks. Currently, this would fail because the NDK directory is being relied on to be not null already when just configuring (not executing) the tasks.

Copy link
Contributor

@artwyman artwyman left a comment

Choose a reason for hiding this comment

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

LGTM if it works. @xianwen can you take it from here?

@sschuberth
Copy link
Contributor Author

Ping @xianwen!

def ndkBuildExecutable = new File(ndkDir, 'ndk-build')
def ndkDir = project.android.ndkDirectory
if (ndkDir) {
def ndkBuildExecutable = new File(ndkDir, 'ndk-build')
Copy link

@xianwen xianwen Jan 3, 2018

Choose a reason for hiding this comment

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

This ndkBuildExecutable here is also used below in L50, wrapping it in this if statement makes it invisible outside and causes error when executing if (!ndkBuildExecutable.exists()) { in L50.

@xianwen
Copy link

xianwen commented Jan 3, 2018

My apologies for the delayed review, there were too many things going on for me during last month. Hopefully I'll be able to respond sooner from now on.

Each version of the Android Gradle Plugin now has a default version of the
build tools. The "compile" configuration was renamed to "implementation".
@sschuberth
Copy link
Contributor Author

Thanks for the review @xianwen and for pointing out that valid issue. I've fixed that plus made some updates to the other commits.

@xianwen xianwen merged commit 565425d into dropbox:master Jan 4, 2018
@sschuberth sschuberth deleted the upgrade-example-build branch January 4, 2018 20:12
mariannegru added a commit to mariannegru/djinni that referenced this pull request Jan 19, 2018
* upstream/master: (54 commits)
  Upgrade example build (dropbox#339)
  Separate `flags` to `enum_flags.djinni` (dropbox#346)
  Add a script to build static fat binary lib. (dropbox#341)
  Upgrade the example to Gradle 4.2 and Android plugin 2.3.3 (dropbox#329)
  Android parcelable support (dropbox#308)
  Add assembly to produce standalone jar. (dropbox#324)
  New "flags" archetype for enums oriented towards bitwise flags (dropbox#323)
  Add Xianwen to contacts
  Fix CFStringEncoding/NSStringEncoding confusion
  Added missing std:: in platform_threads_impl (dropbox#321)
  Avoid unused variable warning when compiling djinni_support.cpp (dropbox#319)
  [Android] Fix Djinni initialization crash in multi shared library case (dropbox#310)
  Fix rsync commands to not update timestamps on unchanged contents
  Swap the README.md change for a CONTRIBUTORS file for attribution.
  Renaming "temp" to "proxy_handle" to make more readable.
  Adding comment to warn of the potential segfault in gcc.
  Making three changes to fix compilation in our environment.
  Make Swift code more idiomatic
  move CMakeLists.txt file to root directory and rename target name to djinni_support_lib
  revert unintended generator change
  ...

# Conflicts:
#	example/generated-src/objc/TXSTextboxListener+Private.mm
#	example/objc/TextSort.xcodeproj/project.pbxproj
#	example/objc/TextSort.xcodeproj/xcshareddata/xcschemes/TextSort.xcscheme
#	src/source/ObjcppGenerator.scala
#	src/source/parser.scala
#	test-suite/djinni/common.djinni
#	test-suite/generated-src/inFileList.txt
#	test-suite/generated-src/objc/DBClientInterface+Private.mm
#	test-suite/generated-src/objc/DBEnumUsageInterface+Private.mm
#	test-suite/generated-src/objc/DBExternInterface2+Private.mm
#	test-suite/generated-src/objc/DBFirstListener+Private.mm
#	test-suite/generated-src/objc/DBObjcOnlyListener+Private.mm
#	test-suite/generated-src/objc/DBSecondListener+Private.mm
#	test-suite/generated-src/objc/DBUserToken+Private.mm
#	test-suite/generated-src/objc/DBUsesSingleLanguageListeners+Private.mm
#	test-suite/generated-src/outFileList.txt
#	test-suite/handwritten-src/java/com/dropbox/djinni/test/AllTests.java
#	test-suite/objc/DjinniObjcTest.xcodeproj/project.pbxproj
KhalilBellakrid pushed a commit to KhalilBellakrid/djinni that referenced this pull request Mar 8, 2018
* example: Guard against ndkDir being null at configuration time

This is a follow-up to PR dropbox#329.

* example: Upgrade to Android Gradle plugin 3.0.1

Each version of the Android Gradle Plugin now has a default version of the
build tools. The "compile" configuration was renamed to "implementation".

* example: Upgrade to Gradle 4.4.1
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants