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.

Sign up for GitHub

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

Jump to bottom

Bump gl-native to 10.4.0-beta.1, mapbox-common to v21.2.0-beta.1 #1160

Merged
merged 1 commit into from
Feb 21, 2022
Merged

Bump gl-native to 10.4.0-beta.1, mapbox-common to v21.2.0-beta.1 #1160

merged 1 commit into from
Feb 21, 2022

Conversation

Chaoba
Copy link
Contributor

@Chaoba Chaoba commented Feb 15, 2022 •
edited
Loading

Summary of changes

User impact (optional)

Pull request checklist:

  • Briefly describe the changes in this PR.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality. If tests were not written, please explain why.
  • Optimize code for java consumption (@JvmOverloads, @file:JvmName, etc).
  • Add example if relevant.
  • Document any changes to public APIs.
  • Run ./gradlew apiDump to update generated api files, if there's public API changes, otherwise the verify-kotlin-binary-compatibility CI will fail.
  • Apply changelog label ('breaking change', 'bug 🪲', 'build', 'docs', 'feature 🍏', 'performance ⚡', 'testing 💯') or use the label 'skip changelog'
  • Add an entry inside this element for inclusion in the mapbox-maps-android changelog: <changelog></changelog>.
  • If this PR is a v10.[version] release branch fix / enhancement, merge it to main firstly and then port to v10.[version] release branch.

Fixes: < Link to related issues that will be fixed by this pull request, if they exist >

PRs must be submitted under the terms of our Contributor License Agreement CLA.

@Chaoba Chaoba added the dependencies Pull requests that update a dependency file label Feb 15, 2022
@Chaoba Chaoba self-assigned this Feb 15, 2022
@Chaoba Chaoba requested a review from a team as a code owner February 15, 2022 04:11
kiryldz
kiryldz reviewed Feb 15, 2022
View reviewed changes
LICENSE.md Outdated Show resolved Hide resolved
@kiryldz
Copy link
Contributor

kiryldz commented Feb 15, 2022

@Chaoba api file should be updated to reflect new hasStyleImage public API. Also a lot of checks are red - I guess we do need merge updated AGP and then rebase this PR.

@Chaoba
Copy link
Contributor Author

Chaoba commented Feb 15, 2022

Need merge #1118 first.

@kiryldz That's correct

@Chaoba Chaoba force-pushed the kl-bump10.4 branch 3 times, most recently from 2bb695b to 38eed64 Compare February 18, 2022 10:10
kiryldz
kiryldz reviewed Feb 18, 2022
View reviewed changes
extension-style-app/build.gradle.kts
}
jniLibs.pickFirsts.add("**/libmapbox-common.so")
Copy link
Contributor

Choose a reason for hiding this comment

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

why is it needed and even if it's needed - shouldn't it be inside if above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without this config there will be two libmapbox-common.so and fails the build

Copy link
Contributor

Choose a reason for hiding this comment

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

@Chaoba why was it working before? is it related to gradle update?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it is related to this bump. Not exactly sure why.

Copy link
Member

Choose a reason for hiding this comment

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

Could it be related to the gl-native gradle version bump? cc @ank27

Copy link
Member

Choose a reason for hiding this comment

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

Actually this is a regression from gl-native, and it will break our customers as well, just downloaded the android-core-10.4.0-beta.1.aar and it contains libmapbox-common.so, which shouldn't be the case, since we have a separate common dependency that has the same libmapbox-common.so.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could it be related to the gl-native gradle version bump? cc @ank27
It shouldn't be there. it could be related to the version bump but need to check further.

kiryldz
kiryldz reviewed Feb 18, 2022
View reviewed changes
Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

@Chaoba can you reach to gl-native team and ask for better public changelog notes? IMO 70% of entries are very internal and simply don't bring any value (e.g. MapRecorder is C++ only).
Also we put . in the end of each changelog entry.

@Chaoba Chaoba force-pushed the kl-bump10.4 branch 2 times, most recently from 9a2bd6e to 1d16eed Compare February 18, 2022 12:37
@Chaoba Chaoba requested a review from kiryldz February 18, 2022 12:39
kiryldz
kiryldz approved these changes Feb 21, 2022
View reviewed changes
Copy link
Contributor

@kiryldz kiryldz left a comment

Choose a reason for hiding this comment

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

LGTM with several questions

pengdev
pengdev approved these changes Feb 21, 2022
View reviewed changes
Copy link
Member

@pengdev pengdev left a comment

Choose a reason for hiding this comment

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

LGTM, please ticket out the tailwork to bump to gl-native v10.4.0-beta.2 before we cut the Android SDK beta release.

@Chaoba Chaoba merged commit 1afe8e7 into main Feb 21, 2022
@Chaoba Chaoba deleted the kl-bump10.4 branch February 21, 2022 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ank27 ank27 ank27 left review comments

@pengdev pengdev pengdev approved these changes

@kiryldz kiryldz kiryldz approved these changes

Assignees

@Chaoba Chaoba

Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Chaoba @kiryldz @pengdev @ank27