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

deps: V8: cherry-pick 8ec2651fbdd8 #49862

Merged
merged 2 commits into from
Sep 27, 2023
Merged

deps: V8: cherry-pick 8ec2651fbdd8 #49862

merged 2 commits into from
Sep 27, 2023

Conversation

abmusse
Copy link
Contributor

@abmusse abmusse commented Sep 25, 2023

Original commit message:

fix: EmbeddedTargetOs on IBM i with Python 3.9

For some context, Python 3.9 on IBM i returns "os400" for sys.platform
instead of "aix". We used to build with Python 3.6 which returned "aix"
as the platform

When attempting to build Node.js with python 3.9 on IBM i we run into a
build error.

Ref: https://github.com/nodejs/node/pull/48056
Ref: https://github.com/nodejs/node/pull/48056#issuecomment-1553719508

I'm not quite sure where target_os is being passed down to the function ToEmbeddedTargetOs.
It seems as though target_os is being generated from sys.platform or
similar call from python as we started running into this issue after
building with Python 3.9.

This PR supersedes initial changes proposed in:
https://chromium-review.googlesource.com/c/v8/v8/+/4259330

This PR contains the minimal changes to successfully build Node.js (builds v8 as an internal dep)
on IBM i with Python 3.9.

Change-Id: I32d43197bce994a72a0d85091e91f80eeea4482d
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846413
Commit-Queue: Jakob Linke <jgruber@chromium.org>
Reviewed-by: Michael Achenbach <machenbach@chromium.org>
Reviewed-by: Jakob Linke <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/main@{#89981}

Refs: v8/v8@8ec2651

CC @nodejs/platform-ibmi

@abmusse
deps: V8: cherry-pick 8ec2651fbdd8
f2f81d1 
Original commit message:

    fix: EmbeddedTargetOs on IBM i with Python 3.9

    For some context, Python 3.9 on IBM i returns "os400" for sys.platform
    instead of "aix". We used to build with Python 3.6 which returned "aix"
    as the platform

    When attempting to build Node.js with python 3.9 on IBM i we run into a
    build error.

    Ref: #48056
    Ref: #48056 (comment)

    I'm not quite sure where target_os is being passed down to the function ToEmbeddedTargetOs.
    It seems as though target_os is being generated from sys.platform or
    similar call from python as we started running into this issue after
    building with Python 3.9.

    This PR supersedes initial changes proposed in:
    https://chromium-review.googlesource.com/c/v8/v8/+/4259330

    This PR contains the minimal changes to successfully build Node.js (builds v8 as an internal dep)
    on IBM i with Python 3.9.

    Change-Id: I32d43197bce994a72a0d85091e91f80eeea4482d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846413
    Commit-Queue: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Michael Achenbach <machenbach@chromium.org>
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89981}

Refs: v8/v8@8ec2651
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/v8-update

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency. labels Sep 25, 2023
@abmusse abmusse mentioned this pull request Sep 25, 2023
Closed
@richardlau
Copy link
Member

richardlau commented Sep 25, 2023 •
edited
Loading

hmm @abmusse did common.gypi get updated on your branch (bumping the v8_embedder_string? It's possible that its not showing as a change here in this PR because #49703 landed today, which also bumped that string.

@abmusse
Copy link
Contributor Author

abmusse commented Sep 25, 2023

Hmm I'm not seeing common.gypi updated on my branch.

After backporting the v8 commit with:

git node v8 backport '8ec2651'

I did rebase the changes on main before pushing the commit up.

git pull --rebase origin main

@richardlau
Copy link
Member

ah, the rebase would have realized that the common.gypi change was already done in an upstream commit to main (which makes the change in your commit a no-op so it's not included). Could you push a change to bump the number of v8_embedder_string by one?

@abmusse
Copy link
Contributor Author

abmusse commented Sep 25, 2023

ah, the rebase would have realized that the common.gypi change was already done in an upstream commit to main (which makes the change in your commit a no-op so it's not included). Could you push a change to bump the number of v8_embedder_string by one?

Thanks for heads up! I bumped this up in 6a3bcee

@richardlau richardlau added commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. request-ci Add this label to start a Jenkins CI on a PR. labels Sep 25, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54222/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=benchmark-ubuntu2204-intel-64,v8test=v8test/5584/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-s390x,v8test=v8test/5584/

@nodejs-github-bot
Copy link
Collaborator

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/nodes=rhel8-ppc64le,v8test=v8test/5584/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54235/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/54248/

@github-actions github-actions bot mentioned this pull request Sep 27, 2023
Open
21 tasks
@debadree25 debadree25 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 27, 2023
mhdawson
mhdawson approved these changes Sep 27, 2023
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@richardlau richardlau added the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Sep 27, 2023
@nodejs-github-bot nodejs-github-bot merged commit 3838b57 into nodejs:main Sep 27, 2023
33 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 3838b57

@abmusse abmusse deleted the backport-v8-patch-ibmi-with-python39 branch September 27, 2023 19:33
@github-actions github-actions bot mentioned this pull request Sep 28, 2023
Open
19 tasks
@RafaelGSS RafaelGSS mentioned this pull request Oct 9, 2023
Merged
alexfernandez pushed a commit to alexfernandez/node that referenced this pull request Nov 1, 2023
@abmusse @alexfernandez
deps: V8: cherry-pick 8ec2651fbdd8
fc0565a 
Original commit message:

    fix: EmbeddedTargetOs on IBM i with Python 3.9

    For some context, Python 3.9 on IBM i returns "os400" for sys.platform
    instead of "aix". We used to build with Python 3.6 which returned "aix"
    as the platform

    When attempting to build Node.js with python 3.9 on IBM i we run into a
    build error.

    Ref: nodejs#48056
    Ref: nodejs#48056 (comment)

    I'm not quite sure where target_os is being passed down to the function ToEmbeddedTargetOs.
    It seems as though target_os is being generated from sys.platform or
    similar call from python as we started running into this issue after
    building with Python 3.9.

    This PR supersedes initial changes proposed in:
    https://chromium-review.googlesource.com/c/v8/v8/+/4259330

    This PR contains the minimal changes to successfully build Node.js (builds v8 as an internal dep)
    on IBM i with Python 3.9.

    Change-Id: I32d43197bce994a72a0d85091e91f80eeea4482d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846413
    Commit-Queue: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Michael Achenbach <machenbach@chromium.org>
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89981}

Refs: v8/v8@8ec2651
PR-URL: nodejs#49862
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
targos pushed a commit that referenced this pull request Nov 11, 2023
@abmusse @targos
deps: V8: cherry-pick 8ec2651fbdd8
7ca1228 
Original commit message:

    fix: EmbeddedTargetOs on IBM i with Python 3.9

    For some context, Python 3.9 on IBM i returns "os400" for sys.platform
    instead of "aix". We used to build with Python 3.6 which returned "aix"
    as the platform

    When attempting to build Node.js with python 3.9 on IBM i we run into a
    build error.

    Ref: #48056
    Ref: #48056 (comment)

    I'm not quite sure where target_os is being passed down to the function ToEmbeddedTargetOs.
    It seems as though target_os is being generated from sys.platform or
    similar call from python as we started running into this issue after
    building with Python 3.9.

    This PR supersedes initial changes proposed in:
    https://chromium-review.googlesource.com/c/v8/v8/+/4259330

    This PR contains the minimal changes to successfully build Node.js (builds v8 as an internal dep)
    on IBM i with Python 3.9.

    Change-Id: I32d43197bce994a72a0d85091e91f80eeea4482d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846413
    Commit-Queue: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Michael Achenbach <machenbach@chromium.org>
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89981}

Refs: v8/v8@8ec2651
PR-URL: #49862
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
@targos targos mentioned this pull request Nov 12, 2023
Merged
@juanarbol juanarbol mentioned this pull request Dec 6, 2023
Merged
debadree25 pushed a commit to debadree25/node that referenced this pull request Apr 15, 2024
@abmusse @debadree25
deps: V8: cherry-pick 8ec2651fbdd8
6575fac 
Original commit message:

    fix: EmbeddedTargetOs on IBM i with Python 3.9

    For some context, Python 3.9 on IBM i returns "os400" for sys.platform
    instead of "aix". We used to build with Python 3.6 which returned "aix"
    as the platform

    When attempting to build Node.js with python 3.9 on IBM i we run into a
    build error.

    Ref: nodejs#48056
    Ref: nodejs#48056 (comment)

    I'm not quite sure where target_os is being passed down to the function ToEmbeddedTargetOs.
    It seems as though target_os is being generated from sys.platform or
    similar call from python as we started running into this issue after
    building with Python 3.9.

    This PR supersedes initial changes proposed in:
    https://chromium-review.googlesource.com/c/v8/v8/+/4259330

    This PR contains the minimal changes to successfully build Node.js (builds v8 as an internal dep)
    on IBM i with Python 3.9.

    Change-Id: I32d43197bce994a72a0d85091e91f80eeea4482d
    Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/4846413
    Commit-Queue: Jakob Linke <jgruber@chromium.org>
    Reviewed-by: Michael Achenbach <machenbach@chromium.org>
    Reviewed-by: Jakob Linke <jgruber@chromium.org>
    Cr-Commit-Position: refs/heads/main@{#89981}

Refs: v8/v8@8ec2651
PR-URL: nodejs#49862
Reviewed-By: Richard Lau <rlau@redhat.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@richardlau richardlau richardlau approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abmusse @nodejs-github-bot @richardlau @mhdawson @debadree25