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

update re2 build for arm under node 14 #1454

Merged
merged 1 commit into from
Apr 15, 2022
Merged

update re2 build for arm under node 14 #1454

merged 1 commit into from
Apr 15, 2022

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Apr 13, 2022

Description

We build and restore re2 for arm. With a license built in,
we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Screen Shot 2022-04-13 at 14 29 40

Issues Resolved

#1436

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@ananzh ananzh requested a review from a team as a code owner April 13, 2022 21:56
kavilla
kavilla previously approved these changes Apr 14, 2022
View reviewed changes
@kavilla kavilla added the v2.0.0 label Apr 14, 2022
@kavilla kavilla linked an issue Apr 14, 2022 that may be closed by this pull request
[BUG] Re2 Requires a higher NODE_MODULE_VERSION #1436
Closed
@kavilla kavilla added the bug Something isn't working label Apr 14, 2022
@ananzh
Copy link
Member Author

ananzh commented Apr 14, 2022

According to the feedbacks, I will add a unit test here. It will cover two things:
1)PatchNativeModules could download the correct package
2)PatchNativeModules could differentiate .tar.gz and .gz and unzip the package correctly.

@ananzh ananzh mentioned this pull request Apr 14, 2022
Merged
7 tasks
kavilla
kavilla previously approved these changes Apr 14, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

tmarkley
tmarkley reviewed Apr 14, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Thanks for adding the tests Anan! I just have a couple minor suggestions.

src/dev/build/tasks/patch_native_modules_task.test.ts Outdated
jest.clearAllMocks();
});

it('patch_native_modules task downloads the correct package', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: patch_native_modules is inferred because of the file name. You could also be specific about what correct means here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I change patch_native_modules task to patch native modules task . Seems its name already explain what it is doing. Then I specify correct to correct platform to show platform is the concern.

src/dev/build/tasks/patch_native_modules_task.test.ts Outdated
Comment on lines 43 to 50

const build = new Build(config);

download.mockImplementation(() => {});

untar.mockImplementation(() => {});

gunzip.mockImplementation(() => {});

return { config, build };
Copy link
Contributor

Choose a reason for hiding this comment

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

very minor: can we remove the extra whitespace here?

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed line 47 and 49

@ananzh
update re2 build for arm under node 14
213e2ca 
We build and restore re2 for arm. With a license built in,
we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Commit-2: add a unit test for PatchNativeModules
Commit-3: polish the code and delete whitespaces

Issue Resolved:
opensearch-project#1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@tmarkley tmarkley merged commit 0e92904 into opensearch-project:main Apr 15, 2022
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 19, 2022
@ananzh
update re2 build for arm under node 10
9ef46a4 
This is a backport for the fix we made on 2.0. Please check
the Backport PR for more details. The difference between this
version and 2.0 is node bump.

From originial PR: We build and restore re2 for arm. With a license
built in, we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Backport PR: opensearch-project#1454
Issue Resolved: opensearch-project#1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh mentioned this pull request Apr 19, 2022
Closed
7 tasks
ananzh added a commit to ananzh/OpenSearch-Dashboards that referenced this pull request Apr 20, 2022
@ananzh
[1.x] update re2 build for arm under node 10
284ac14 
This is a backport for the fix we made on 2.0. Please check
the Backport PR for more details. The difference between this
version and 2.0 is node bump.

From originial PR: We build and restore re2 for arm. With a license
built in, we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Backport PR: opensearch-project#1454
Issue Resolved: opensearch-project#1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh mentioned this pull request Apr 20, 2022
Merged
7 tasks
tmarkley pushed a commit that referenced this pull request Apr 28, 2022
@ananzh
[1.x] update re2 build for arm under node 10 (#1482)
d0f368a 
This is a backport for the fix we made on 2.0. Please check
the Backport PR for more details. The difference between this
version and 2.0 is node bump.

From originial PR: We build and restore re2 for arm. With a license
built in, we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Backport PR: #1454
Issue Resolved: #1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 28, 2022
@ananzh @github-actions
[1.x] update re2 build for arm under node 10 (#1482)
bb294a7 
This is a backport for the fix we made on 2.0. Please check
the Backport PR for more details. The difference between this
version and 2.0 is node bump.

From originial PR: We build and restore re2 for arm. With a license
built in, we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Backport PR: #1454
Issue Resolved: #1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
(cherry picked from commit d0f368a)
tmarkley pushed a commit that referenced this pull request Apr 28, 2022
@opensearch-trigger-bot
[1.x] Update re2 build for arm under node 10 (#1482) (#1510)
208a9ae 
This is a backport for the fix we made on 2.0. Please check
the Backport PR for more details. The difference between this
version and 2.0 is node bump.

From originial PR: We build and restore re2 for arm. With a license
built in, we zip the build artifact as .tar.gz. The original patchModule
method has a default extract method and path which causes issues
for extracting and using re2 arm build artifact. Therefore, we
modify the method in this PR by passing an overriddenExtractMethod
and an overriddenDestinationPath.

Backport PR: #1454
Issue Resolved: #1436

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
(cherry picked from commit d0f368a)
@ananzh ananzh deleted the re2-arm branch June 1, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmarkley tmarkley tmarkley approved these changes

@kavilla kavilla kavilla approved these changes

@boktorbb boktorbb Awaiting requested review from boktorbb

Assignees
No one assigned
Labels
bug Something isn't working v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] Re2 Requires a higher NODE_MODULE_VERSION
3 participants
@ananzh @tmarkley @kavilla