FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: zlib inlining failed fix for IA-32 by RaisinTen · Pull Request #35679 · nodejs/node · GitHub

/ node Public

fix: zlib inlining failed fix for IA-32 - #35679

Merged
Trott merged 0 commit into
nodejs:masterfrom
RaisinTen:zlib-inlining-error-fix
Nov 9, 2020
ConversationCommits (0)ChecksFiles changed
Merged

fix: zlib inlining failed fix for IA-32#35679
Trott merged 0 commit into
nodejs:masterfrom
RaisinTen:zlib-inlining-error-fix

Conversation

RaisinTen commented Oct 16, 2020

Copy link
Copy Markdown
Member

closes:

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 16, 2020

Trott commented Oct 16, 2020

Copy link
Copy Markdown
Member

Welcome, @RaisinTen, and thanks for the pull request!

If possible, we'll want to avoid floating an additional patch on a dependency like this, especially for unsupported platforms. I think it would be better to first get #33044 land-able, and then perhaps update zlib.h per doc/guides/maintaining-zlib.md. I don't know if that will fix your issue or not, but regardless, I think we want to do that before patching a dependency like this.

RaisinTen commented Oct 16, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

Hello @Trott. 🙂

Going through the PR, I understand that we are going to update the zlib-chromium fork regularly. In that case, my change, which is basically a change to the zlib files would end up getting erased on updates. So, I should add my change to deps/zlib/zlib.gyp probably which the PR author was working on. I'd love to help but I'm not sure how I can make changes to his PR to help land it. What should I do?

Trott commented Oct 16, 2020

Copy link
Copy Markdown
Member

@nodejs/zlib Thoughts on this? I guess we can land this and then it will be over-written if #33044 ever lands. But there's a danger that will be stalled for a long time.

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020
github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020

nodejs-github-bot commented Oct 16, 2020

Copy link
Copy Markdown
Collaborator

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

addaleax approved these changes Oct 17, 2020
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I don’t really see how this could hurt, so yeah, if it passes CI I’m good with it :)

RaisinTen commented Nov 5, 2020

Copy link
Copy Markdown
Member Author

Hey @Trott, do you know when we can land this PR? It's taking a long time, so I was wondering.

Trott closed this Nov 9, 2020
Trott force-pushed the zlib-inlining-error-fix branch from 201189c to ab587ca Compare November 9, 2020 20:37
Trott merged commit ab587ca into nodejs:master Nov 9, 2020

Trott commented Nov 9, 2020

Copy link
Copy Markdown
Member

Landed in ab587ca.

RaisinTen deleted the zlib-inlining-error-fix branch November 10, 2020 01:45
danielleadams pushed a commit that referenced this pull request Nov 10, 2020
build: fix zlib inlining for IA-32
10c9ea7 
PR-URL: #35679
Fixes: #35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
danielleadams mentioned this pull request Nov 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
build: fix zlib inlining for IA-32
a4a086d 
PR-URL: #35679
Fixes: #35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs pushed a commit that referenced this pull request Dec 10, 2020
build: fix zlib inlining for IA-32
200812e 
PR-URL: #35679
Fixes: #35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
build: fix zlib inlining for IA-32
f498127 
PR-URL: #35679
Fixes: #35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
RaisinTen mentioned this pull request Dec 30, 2020
Closed
2 tasks
RaisinTen added a commit to RaisinTen/node that referenced this pull request Jan 1, 2021
build: add TODO comments to add -msse2 for zlib
b35ae3b 
Refs: nodejs#35679
targos pushed a commit to targos/node that referenced this pull request Jan 29, 2022
build: fix zlib inlining for IA-32
ec258fe 
PR-URL: nodejs#35679
Fixes: nodejs#35629
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Rich Trott <rtrott@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott approved these changes
addaleax addaleax approved these changes

Assignees

No one assigned

Labels

zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL