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

refactor Android Support Library in Buck #22096

Closed
wants to merge 2 commits into from

Conversation

dulmandakh
Copy link
Contributor

@dulmandakh dulmandakh commented Nov 3, 2018

Refactor Android Support Library in Buck

Rationale:

Many PRs involving Android Support Library were failed to import, even though Android CI is green (#20602, #19768, #20621). Also exports from FB to GitHub broke Android CI and fixed with 99632e1, 1edeaef.

After some investigation and help from FB developers, I found that BUCK target names for Android Support Library are different in GitHub and FB repos. Also there might be a software tool that translates/maps FB's Android Support Library dependencies into something like below in GitHub. But it's not working properly and results in cannot find symbol errors.

provided_deps = [
    react_native_dep("third-party/android/support/v4:lib-support-v4"),
],

And this issue is blocking many improvements involving Android Support Library, and should be resolved as soon as possible.

Proposed solution:

Instead of translating/mapping Android Support Library target names in BUCK dependencies, which might break anytime, why not have same target names in both FB and GitHub, therefore have Android Support Library BUCK files to have different contents in FB and GitHub, due to FB internal requirements. This requires less mapping and less error prone.

Here are proposed steps to make it work.

  1. Merge this PR, and ASL Buck files separate. But must have same target paths.
  2. Remove Android Support Library target mappings as dependency and export BUCK files to GitHub.
  3. Remove old Android Support Library target names from GitHub once we confirm step 2 worked, which was kept to make transition smoother.

Test Plan:

CI is green

@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 Nov 3, 2018
@pull-bot
Copy link

pull-bot commented Nov 3, 2018

Warnings
⚠️

📋 Changelog - This PR appears to be missing Changelog.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added the Platform: Android Android applications. label Nov 3, 2018
@dulmandakh dulmandakh requested a review from hramos November 3, 2018 12:09
@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 Nov 6, 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.

@hramos
Copy link
Contributor

hramos commented Nov 7, 2018

Failed with The rule //third-party/android/support:all-support-v4 could not be found. and No build file at third-party/android/support/BUCK when resolving target fbandroid//third-party/android/support:support-annotations.

@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 Nov 7, 2018
@dulmandakh
Copy link
Contributor Author

@mdvacca @hramos I added some comments about the PR to make its intentions clear.

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.

@mdvacca has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@hramos hramos added the Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. label Jan 15, 2019
@dulmandakh
Copy link
Contributor Author

closing in favor of #22962

@dulmandakh dulmandakh closed this Jan 16, 2019
@dulmandakh dulmandakh deleted the buck-asl branch January 16, 2019 10:22
@hramos hramos added Merged This PR has been merged. and removed Import Failed labels Mar 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants