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

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

Merged
merged 0 commits into from
Nov 9, 2020
Merged

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

merged 0 commits into from
Nov 9, 2020

Conversation

RaisinTen
Copy link
Contributor

closes:

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

@nodejs-github-bot nodejs-github-bot added the zlib Issues and PRs related to the zlib subsystem. label Oct 16, 2020
@Trott
Copy link
Member

Trott commented Oct 16, 2020

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
Copy link
Contributor Author

RaisinTen commented Oct 16, 2020 •
edited
Loading

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
Copy link
Member

Trott commented Oct 16, 2020

@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 Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 16, 2020
@github-actions 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
Copy link
Collaborator

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

addaleax
addaleax approved these changes Oct 17, 2020
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

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

@RaisinTen
Copy link
Contributor Author

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

@Trott Trott closed this Nov 9, 2020
@Trott Trott merged commit ab587ca into nodejs:master Nov 9, 2020
@Trott
Copy link
Member

Trott commented Nov 9, 2020

Landed in ab587ca.

@RaisinTen 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
@RaisinTen @danielleadams
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 danielleadams mentioned this pull request Nov 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 9, 2020
@RaisinTen @BethGriggs
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
@RaisinTen @BethGriggs
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 BethGriggs mentioned this pull request Dec 10, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Dec 15, 2020
@RaisinTen @BethGriggs
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 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
@RaisinTen
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
@RaisinTen @targos
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@Trott Trott Trott 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
@RaisinTen @Trott @nodejs-github-bot @addaleax