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

Upgrade to soloader 0.5.1 #20325

Closed
wants to merge 4 commits into from

Conversation

gengjiawen
Copy link
Contributor

@gengjiawen gengjiawen commented Jul 20, 2018

Motivation

Upgrade to soloader 0.5.1. Fixes #20323.

Test Plan

pass all current ci.

Related PRs

none

Release Notes

[GENERAL] [BUGFIX] [ANDROID] - Upgrade to soloader 0.5.1

@gengjiawen
Copy link
Contributor Author

@hramos

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 21, 2018
@gengjiawen
Copy link
Contributor Author

Looks like 0.5.1 has some problem, see facebook/SoLoader#10.

@passy
Copy link
Member

passy commented Jul 21, 2018

Can you post the error you're seeing? Again, SoLoader in isolation is fine. It's mixing with other dependencies is fine. Can you check with ./gradle :<target>:dependencies that there is no transitive dependency on a prior version?

@gengjiawen
Copy link
Contributor Author

gengjiawen commented Jul 21, 2018

I post it in this issue facebook/SoLoader#10.
See full log: https://circleci.com/gh/gengjiawen/react-native/676?utm_campaign=build-failed&utm_medium=email&utm_source=notification.

Warning: com.facebook.imagepipeline.nativecode.ImagePipelineNativeLoader: can't find referenced method 'void loadLibrary(java.lang.String)' in program class com.facebook.soloader.SoLoader

@gengjiawen
Copy link
Contributor Author

Do you think it's a issue in fresco. They need a new release after bump soloader version.

@passy
Copy link
Member

passy commented Jul 21, 2018 via email

@gengjiawen
Copy link
Contributor Author

@oprisnik we may need a new fresco release, can you help on this, thanks.

@passy
Copy link
Member

passy commented Jul 23, 2018

@gengjiawen Fresco 1.10.0 is already depending on SoLoader 0.5.0, so this isn't the cause: https://mvnrepository.com/artifact/com.facebook.fresco/fresco/1.10.0

@gengjiawen
Copy link
Contributor Author

But gradle will only use the high version if there are two versions in deps. But let's wait fresco release a new version.

@passy
Copy link
Member

passy commented Jul 23, 2018

@gengjiawen I don't follow. Fresco is using the right version.

@gengjiawen
Copy link
Contributor Author

So you are saying 0.5.0 is clean version ?

@passy
Copy link
Member

passy commented Jul 23, 2018

I don't think there's a "dirty" version. :D But 0.5.0 is the version that introduced the breaking change, so all dependencies need to be either < 0.5.0 or >= 0.5.0.

@passy
Copy link
Member

passy commented Jul 23, 2018

To reiterate: Upgrading Fresco to the already released 1.10.0 should take care of the issue unless there's another dependency that was built against an old version.

@gengjiawen
Copy link
Contributor Author

@passy If that's the case. 0.5.1 has bug, the proof is pretty clear.

@passy
Copy link
Member

passy commented Jul 23, 2018

Why?

@gengjiawen
Copy link
Contributor Author

Are you an android developer ? I think the build log is pretty clear.

@passy
Copy link
Member

passy commented Jul 23, 2018

@gengjiawen That's not a very helpful comment. I'm asking why you insist that SoLoader is broken while everything is pointing to the old version of Fresco which is still in the manifest being responsible for the breakage.

@gengjiawen
Copy link
Contributor Author

@passy I see your point, let's try new fresco version and see what happens.

@gengjiawen
Copy link
Contributor Author

gengjiawen commented Jul 23, 2018

@passy The build passed. It seems it's fresco issue. The comment I meant is that from proguard it's definitely soloader issue. I thought maybe you not an android developer so you missed the part. Sorry if you feel offended.

@gengjiawen
Copy link
Contributor Author

@hramos I think this can be merged.

@passy
Copy link
Member

passy commented Jul 23, 2018

I think it would be good to have the buck dependencies updated in the same commit to prevent them from diverging.

@gengjiawen
Copy link
Contributor Author

@passy Add buck will be good. But I am not familiar with buck. And also we have fix the issue.
So maybe deal with later or can you help on this ?

@passy
Copy link
Member

passy commented Jul 23, 2018

I could have with that tomorrow. I'll leave it to the maintainers if they want to wait with merging till then.

@gengjiawen
Copy link
Contributor Author

Glad to hear that.

@hramos hramos changed the title Fix 20323 Upgrade to soloader 0.5.1 Jul 23, 2018
@gengjiawen gengjiawen requested a review from hramos July 24, 2018 03:17
@passy
Copy link
Member

passy commented Jul 24, 2018

Added a commit to update the Buck targets.

@gengjiawen
Copy link
Contributor Author

looks like it broken android build.

@gengjiawen
Copy link
Contributor Author

I see, the master branch commit cause this problem, then we are fine.

@dulmandakh
Copy link
Contributor

@gengjiawen Android CI is green again, please rebase

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 30, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@gengjiawen
Copy link
Contributor Author

@dulmandakh Any special reason you want to bump findbugs version ?

@facebook-github-bot
Copy link
Contributor

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

@facebook-github-bot facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jul 31, 2018
@dulmandakh
Copy link
Contributor

dulmandakh commented Jul 31, 2018 via email

@gengjiawen
Copy link
Contributor Author

looks like this cause import failed. And this could be done via new pr. Not mixing with this one.

@dulmandakh
Copy link
Contributor

@gengjiawen maybe you should wait, because failure was caused by conflict with previously merged PRs.

@facebook-github-bot shipit

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jul 31, 2018
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

dulmandakh is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was closed by @gengjiawen in b6f2aad.

Once this commit is added to a release, you will see the corresponding version tag below the description at b6f2aad. If the commit has a single master tag, it is not yet part of a release.

@facebook facebook locked as resolved and limited conversation to collaborators Jul 31, 2018
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jul 31, 2018
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. Import Failed labels Feb 6, 2019
@facebook-github-bot facebook-github-bot added the Contributor A React Native contributor. label Apr 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants