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.

Sign up for GitHub

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

Jump to bottom

chore: rename hermes-inspector-msggen to @react-native/hermes-inspector-msggen #34850

Closed
wants to merge 5 commits into from
Closed

chore: rename hermes-inspector-msggen to @react-native/hermes-inspector-msggen #34850

wants to merge 5 commits into from

Conversation

gabrieldonadel
Copy link
Collaborator

Summary

This PR renames hermes-inspector-msggen to @react-native/hermes-inspector-msggen, moves it to packages/hermes-inspector-msggen and makes the package private as part of RFC480.

Related to #34692

Changelog

[General] [Changed] - Move and rename hermes-inspector-msggen to @react-native/hermes-inspector-msggen

Test Plan

run sh ReactCommon/hermes/inspector/tools/run_msggen

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Oct 3, 2022
cortinico
cortinico requested changes Oct 3, 2022
View reviewed changes
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Can you remove the yarn.lock file?

@gabrieldonadel
Copy link
Collaborator Author

Can you remove the yarn.lock file?

Sure, done

cortinico
cortinico reviewed Oct 3, 2022
View reviewed changes
ReactCommon/hermes/inspector/tools/run_msggen Outdated
@@ -12,7 +12,7 @@ FBSOURCE=$(hg root)
CLANG_FORMAT="${FBSOURCE}/tools/third-party/clang-format/clang-format"
SIGNEDSOURCE="${FBSOURCE}/tools/signedsource"

cd "${DIR}/msggen"
cd "${DIR}/../../../../packages/hermes-inspector-msggen"
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we pushd and popd here as the paths are so distant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I've just pushed a commit updating this

@analysis-bot
Copy link

analysis-bot commented Oct 3, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,739,362 -17
android hermes armeabi-v7a 7,142,028 -27
android hermes x86 8,050,472 +8
android hermes x86_64 8,022,301 -25
android jsc arm64-v8a 9,607,549 +57
android jsc armeabi-v7a 8,373,071 +70
android jsc x86 9,555,218 +64
android jsc x86_64 10,147,821 +61

Base commit: 8486b4c
Branch: main

cortinico
cortinico reviewed Oct 4, 2022
View reviewed changes
Copy link
Contributor

@cortinico cortinico left a comment

Choose a reason for hiding this comment

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

Can you address the comments and rebase?

ReactCommon/hermes/inspector/tools/run_msggen Outdated
@@ -2,6 +2,9 @@

set -e

ROOT=$(pwd)
pushd $ROOT >/dev/null || exit
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need || exit as you have a set -e on top

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

ReactCommon/hermes/inspector/tools/run_msggen Outdated
@@ -12,7 +15,8 @@ FBSOURCE=$(hg root)
CLANG_FORMAT="${FBSOURCE}/tools/third-party/clang-format/clang-format"
SIGNEDSOURCE="${FBSOURCE}/tools/signedsource"

cd "${DIR}/msggen"
popd >/dev/null || exit
cd "packages/hermes-inspector-msggen"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest we pushd "${DIR}/msggen" (or $ROOT/packages/hermes-inspector-msggen) and popd at the end of this script so the user is back in the folder where the script was invoked.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, updated

@gabrieldonadel gabrieldonadel force-pushed the feat/move-hermes-msggen branch from 29e4605 to 01159df Compare October 4, 2022 12:55
@facebook-github-bot
Copy link
Contributor

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

@analysis-bot
Copy link

analysis-bot commented Oct 4, 2022 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 108c876
Branch: main

@cortinico cortinico added the Tech: Monorepo For PRs that are related to the monorepo infra label Oct 4, 2022
@cortinico cortinico mentioned this pull request Oct 4, 2022
Closed
11 tasks
@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @gabrieldonadel in 530dae8.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Oct 5, 2022
@gabrieldonadel gabrieldonadel deleted the feat/move-hermes-msggen branch October 5, 2022 11:58
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
@gabrieldonadel @OlimpiaZurek
chore: rename hermes-inspector-msggen to @react-native/hermes-inspect…
b59ea24 
…or-msggen (facebook#34850)

Summary:
This PR renames ` hermes-inspector-msggen` to `react-native/hermes-inspector-msggen`, moves it to  `packages/hermes-inspector-msggen` and makes the package private  as part of RFC480.

Related to facebook#34692

## Changelog

[General] [Changed] - Move and rename `hermes-inspector-msggen` to `react-native/hermes-inspector-msggen`

Pull Request resolved: facebook#34850

Test Plan: run `sh ReactCommon/hermes/inspector/tools/run_msggen`

Reviewed By: mattbfb

Differential Revision: D40060406

Pulled By: cortinico

fbshipit-source-id: e018fd88e8374a69fbd8737fbb9abe7565d4a003
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cortinico cortinico Awaiting requested review from cortinico

Assignees
No one assigned
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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Tech: Monorepo For PRs that are related to the monorepo infra
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gabrieldonadel @analysis-bot @facebook-github-bot @react-native-bot @cortinico