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

Update new_architecture.rb for React-ImageManager #38247

Closed
wants to merge 2 commits into from

Conversation

billnbell
Copy link
Contributor

@billnbell billnbell commented Jul 7, 2023

Summary:

This added React-ImageManager to the use_frameworks! - a lot of rpm modules podspec need this.

Changelog:

[iOS] [FIXED] - Add React-ImageManager path to work with use_frameworks!

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Test Plan:

Should not be breaking - it will add to the header_search_paths React-ImageManager

@facebook-github-bot
Copy link
Contributor

Hi @billnbell!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

@billnbell
Copy link
Contributor Author

I signed the CLA

@analysis-bot
Copy link

analysis-bot commented Jul 7, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,885,449 -2
android hermes armeabi-v7a 7,935,798 +1
android hermes x86 9,281,970 -1
android hermes x86_64 9,185,456 -1
android jsc arm64-v8a 9,473,452 +0
android jsc armeabi-v7a 8,416,807 +0
android jsc x86 9,456,195 -1
android jsc x86_64 9,772,407 -1

Base commit: cbf9408
Branch: main

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@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 7, 2023
@cipolleschi
Copy link
Contributor

Hi there! Thanks for taking the time for doing this.
I think we have also to update the ruby tests, otherwise the CI will stay red. I think this is the test that needs to be updated.

@github-actions github-actions bot added the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 19, 2023
@facebook-github-bot
Copy link
Contributor

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

@billnbell
Copy link
Contributor Author

Hi there! Thanks for taking the time for doing this.
I think we have also to update the ruby tests, otherwise the CI will stay red. I think this is the test that needs to be updated.

Yeah I added that one - let's see what the tests do

@cortinico cortinico removed the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 20, 2023
@cipolleschi
Copy link
Contributor

/rebase

@github-actions github-actions bot added the Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue. label Jul 20, 2023
@billnbell billnbell force-pushed the patch-2 branch 2 times, most recently from 9f2bd81 to 39f06cc Compare July 21, 2023 03:32
@billnbell
Copy link
Contributor Author

OK rebased.

@cipolleschi
Copy link
Contributor

I don't know what's going on with the CI. Main is green, I don't understand why this branch can't build correctly... :/

@cipolleschi
Copy link
Contributor

/rebase

@billnbell
Copy link
Contributor Author

What do you want me to do for /rebase ? I am not getting it.

@billnbell
Copy link
Contributor Author

I merged from Facebook into main and merged that and rebased into patch-2. Should be rebased.

@cipolleschi
Copy link
Contributor

Cool! Now the CI is running properly!

There are some ruby tests failures, which are expected!

You can run ruby tests locally on your machines:

  1. navigate to: react-native/packages/react-native/scripts
  2. run ./run_ruby_tests.sh

This will highlights the failures locally and you should be able to fix the tests and update the PR! ;)

Thanks again for taking the time to work on this!

@billnbell
Copy link
Contributor Author

ok fixed those 2.

@billnbell billnbell force-pushed the patch-2 branch 2 times, most recently from 51c9b3d to e1e1e76 Compare July 29, 2023 04:48
Copy link
Contributor Author

@billnbell billnbell left a comment

Choose a reason for hiding this comment

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

ok good

@cipolleschi
Copy link
Contributor

What do you want me to do for /rebase ? I am not getting it.

I'm sorry! 😓

Leaving a comment /rebase on a React Native PR triggers a github action that automatically rebase the PR on top of main, if possible.

That's why I was leaving those messages: I was asking the CI to rebase to rerun the CI and verify that it gets green!

@facebook-github-bot
Copy link
Contributor

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

fix: test for new arch

Squashed commit of the following:

commit dce55b0
Author: Bill Bell <bbell@exclusiveresorts.com>
Date:   Wed Jul 19 21:48:27 2023 -0600

    fix: test for new arch

commit 51e8515
Author: William Bell <williambell9708@outlook.com>
Date:   Fri Jul 7 15:47:00 2023 -0600

    Update new_architecture.rb for React-ImageManager
@github-actions
Copy link

This pull request was successfully merged by @billnbell in 8a63b91.

When will my fix make it into a release? | Upcoming Releases

@github-actions github-actions bot added the Merged This PR has been merged. label Jul 31, 2023
fortmarek pushed a commit that referenced this pull request Aug 8, 2023
Summary:
This added React-ImageManager to the use_frameworks! - a lot of rpm modules podspec need this.

bypass-github-export-checks

[iOS] [FIXED] - Add React-ImageManager path to work with use_frameworks!

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #38247

Test Plan: Should not be breaking - it will add to the header_search_paths React-ImageManager

Reviewed By: dmytrorykun

Differential Revision: D47593749

Pulled By: cipolleschi

fbshipit-source-id: a66e90707e5fa73573deab1f04e8d8693869a90c
kelset pushed a commit that referenced this pull request Aug 11, 2023
Summary:
This added React-ImageManager to the use_frameworks! - a lot of rpm modules podspec need this.

bypass-github-export-checks

[iOS] [FIXED] - Add React-ImageManager path to work with use_frameworks!

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests

Pull Request resolved: #38247

Test Plan: Should not be breaking - it will add to the header_search_paths React-ImageManager

Reviewed By: dmytrorykun

Differential Revision: D47593749

Pulled By: cipolleschi

fbshipit-source-id: a66e90707e5fa73573deab1f04e8d8693869a90c

chore(releases): improve bump oss script to allow less human errors (#38666)

Summary:
One of the limitations of the existing flow for the release crew is that they need to manually remember to publish all the other packages in the monorepo ahead of a new patch release - this PR modifies the logic for the bump-oss-version script (and makes it available via yarn) so that it will not run if:
* there are git changes lying around
* if some of the packages need a new release

it required a bit of refactoring to extract some portions of the logic from the bump-all-package-versions script, but I think the end result is pretty decent.

## Changelog:

<!-- Help reviewers and the release process by writing your own changelog entry.

Pick one each for the category and type tags:

For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->

[INTERNAL] [CHANGED] - improve bump oss script to allow less human errors

Pull Request resolved: #38666

Test Plan:
* checkout this branch
* comment L54 of bump-oss-version.js (to remove the check on the branch name)
* run `yarn bump-all-updated-packages`, verify that it works and that it detects that some packages have unreleased code
* run `yarn bump-oss-version -t asd -v asd` (the "fake" parameters are needed to pass the yargs check), verify that it will throw an error because it finds a package that has unreleased code

Reviewed By: mdvacca

Differential Revision: D48156963

Pulled By: cortinico

fbshipit-source-id: 2473ad5a84578c5236c905fd9aa9a88113fe8d22

# Conflicts:
#	scripts/publish-npm.js

re-add the file

nit

Revert "Update new_architecture.rb for React-ImageManager (#38247)"

This reverts commit 645264e249f566ce87f6aaf462e029909302fd92.
huntie pushed a commit that referenced this pull request Aug 18, 2023
…72 edition) (#38888)

Co-authored-by: William Bell <williambell9708@outlook.com>
resolved: #38247
resolved: #38666
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. Needs: Repro This issue could be improved with a clear list of steps to reproduce the issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants