-
Notifications
You must be signed in to change notification settings - Fork 319
Fix flutter 3.30 build #240
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
base: master
Are you sure you want to change the base?
Conversation
Refactor Android project configuration and update Gradle configurations
|
flutter 3.29.0 error message: git/background_location-c9ec16ed439443beea6b08e18c1e1869ffc7190a/android/src/main/kotlin/com/almoullim/background_location/LocationUpdatesService.kt:255:19 Class 'kotlin.Unit' was compiled with an incompatible version of Kotlin. The actual metadata version is 2.1.0, but the compiler version 1.8.0 can read versions up to 1.9.0. There's a lot of the same, not just one |
|
@Gladario how was this PR tested? Do you have a set of steps that other contributors can use to work through issues like the above (or maybe try different flutter versions)? |
|
Apparently, the Kotlin default version on Flutter 3.30 is higher than the latest release on the Stable channel. I've done tests using Flutter on the 'main' branch (channel main). Since this is creating some sort of incompatibility, I will perform the tests on the stable version and address any issues. For anyone wanting to test in the same setup that I had:
|
|
Well... What's preventing this PR to be merged? 2 weeks have elapsed already. This is a critical issue. |
Unfortunately, there aren't many maintainers around who are able to respond to things. Right now I think this project is mostly operating with a skeleton crew. I personally don't use this library as often as I used to, so while I plan to use it more in the future, I don't currently have much ability to help test things and don't want to create any new, more critical issues by merging code that I can't be sure wont break more things. Knowing that it works for people on 3.29/3.30 is one thing, but I'd also like to hear from some people on 3.28 or lower to see whether this breaks anything for them. That was my goal with trying to encourage the posting of the steps used to test this. Failing that, it would be great to have more robust CI/CD coverage that meaningfully tests the functionality so that I can rely on that as a source of testing. Even if this does get merged, it would simply join the list of issues that are awaiting a new pub.dev release, which I was never given the keys for. In short it seems as though this project needs a more sustainable system for testing and merging fixes, because I don't think there's enough collective maintainer-time available to have everything be centralized on one core team and/or one person. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
BackgroundLocation.getLocationUpdates() is not triggering any event is android, it is working fine in ios, could you please any one look into this?, thanks! |
|
@Gladario !!!, could you please help here. |
I wonder if I am mistaken here. Given the existence of #209, i suspect that, if a new tag were to be pushed to the repo, there's a chance that the automation kicks in as intended and pulls in the pre-configured credentials and performs a push. This would mean the only blocker is having sufficient testing for this issue to convince myself and/or @BWMuller that this is reliable and safe to merge (since I don't personally use this package anymore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok so that test point release worked, which means that Ali has secrets configured in this repo to push to pub.dev that I cannot see, but will activate and push to pub.dev when a tagged release is present (which i have the access to create/tag).
Apologies for my misconception about the need for pub.dev access and the delay it caused for this issue.
Code review looks largely good to me. Had a couple questions added in review but that's it.
For anyone who has an interest in merging this, The main bottleneck is still testing due to the fact that I am not personally an active user of this project.
If one existing maintainer (9 or more commits to this project's main branch prior to this PR) or ~4 people affected by these fixes can confirm that running this codebase before and after this change fixes the issue described in the PR, I will be a lot more comfortable with merging this without being able to test myself. Confirmations with detailed steps to allow someone else to replicate your testing (or other evidence such as a link to an open source project of yours that is reliant on your own fork), have more weight in my mind than just a comment saying "it works".
oh and this change should resolve merge conflicts
| val newBuildDir: Directory = rootProject.layout.buildDirectory.dir("../../build").get() | ||
| rootProject.layout.buildDirectory.value(newBuildDir) | ||
|
|
||
| subprojects { | ||
| val newSubprojectBuildDir: Directory = newBuildDir.dir(project.name) | ||
| project.layout.buildDirectory.value(newSubprojectBuildDir) | ||
| } | ||
| subprojects { | ||
| project.evaluationDependsOn(":app") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a purpose for adjusting the build dir from ../build to ../../build?
|
|
||
| /** | ||
| Legacy for v1 embedding | ||
| */ | ||
| @SuppressWarnings("deprecation") | ||
| fun registerWith(registrar: PluginRegistry.Registrar) { | ||
| val service = BackgroundLocationService.getInstance() | ||
| service.onAttachedToEngine(registrar.context(), registrar.messenger()) | ||
| registrar.addRequestPermissionsResultListener(service) | ||
| } | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to clarify, this PR introduces the new V2 embeddings right?
|
@MoralCode a good discovery. Now it'll at least allow updating this distribution instead of the fork. Do you have any access to add additional maintainers who can tag releases at all so we can bring in people who use the library for more active maintenance? |
I don't think so, the settings tab 404's for me. If people want access and have a history of quality contributions, adding a comment in #188 may help raise the issue and give it enough weight to potentially catch Ali's eye if he gets any amount of time to check this project. Also, are you able to see the "draft a new release" button on the releases tab? i want to say we both should have the same access but I forget how that all went down. Could you validate that this PR solves the various flutter 3.3+ compatibility issues that various people have brought up? Once I'm confident in it, I can probably cut a new minor release for it. |
Fixes build issues on flutter 3.30
Tested on Flutter 3.30.0-1.0.pre.195 • channel main