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

Proposal/Attempt to remove library desugaring #675

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dpgmedia-gbero
Copy link

We observed crashes with devices running Android API 23.
Initialization of FocalMeterConfiguration would crash.

A thread (#601) in the issue tracker suggested to use of coreLibraryDesugaring but we believe there might be a different option : Remove usage of java.util.function.* in order to get rid of library desugaring.

I'm not sure how to entirely test this but can this be looked at ?

Many thanks,

…on of FocalMeterConfiguration would crash.

A thread (snowplow#601) in the issue tracker suggested to use of coreLibraryDesugaring but this might be avoided.

remove usage of java.util.function.* in order to get rid of library desugaring.
@snowplowcla
Copy link

Thanks for your pull request. Is this your first contribution to a Snowplow open source project? Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://docs.snowplowanalytics.com/docs/contributing/contributor-license-agreement/ to learn more and sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

@snowplowcla snowplowcla added the cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed. label Feb 13, 2024
@matus-tomlein
Copy link
Contributor

matus-tomlein commented Feb 13, 2024

Thanks for the proposal, @dpgmedia-gbero! This makes a lot of sense and I do like it better than our current approach.

But my concern is that this is a breaking change and we would need to wait for another major version of the tracker in order to release this (we have just published version 6 and don't plan to publish version 7 any time soon).

In particular, moving from Consumer<InspectableEvent> to (InspectableEvent) -> Unit? or (InspectableEvent) -> Void? is not the same because the Consumer doesn't care about the return type, but the latter representation does. We can also see in our tests that they can't be compiled using the new API.

Do you see any way that we could make this into a non-breaking change so that we can release this in a v6 update to the tracker?

@dpgmedia-gbero
Copy link
Author

I understand your concern. We decided to disable FocalMeterConfiguration on Android 6 and lower instead to circumvent this issue. You can close this or keep it for reference if you'd foresee to get rid of core library desugaring

@plastiv
Copy link

plastiv commented Apr 7, 2024

Hi @matus-tomlein !

Could you consider to evaluate if desugar_jdk_libs_minimal can be used? Based on description it includes function package snowplow sdk has used. And if you agree to not extend the scope of java standard lib usage that may minimize the bundled dependency.

@matus-tomlein
Copy link
Contributor

matus-tomlein commented Apr 13, 2024

Thank you for the suggestion @plastiv! That makes sense, I'll make sure we evaluate this (contributions are of course welcome too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:no [Auto generated] Snowplow Contributor License Agreement has not been signed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants