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.

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

build: fix zlib tarball generation #32094

Closed
wants to merge 1 commit into from

Conversation

@codebytere
Copy link
Member

codebytere commented Mar 4, 2020

Closes #31858.

Ensures that contrib/optimizations isn't stripped from the source tree to mitigate build failures e.g

../deps/zlib/deflate.c:54:10: fatal error: 'contrib/optimizations/insert_string.h' file not found
#include "contrib/optimizations/insert_string.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I'm not 100% sure we need to include all of contrib/optimizations, so let me know if there are any changes I'll need to make.

cc @MylesBorins @richardlau

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
@codebytere codebytere requested review from MylesBorins and richardlau Mar 4, 2020
@jasnell
jasnell approved these changes Mar 4, 2020
@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 4, 2020

I'm going through and testing locally. Will add my explicit +1 when I can confirm this is fixed

@codebytere

This comment has been minimized.

Copy link
Member Author

codebytere commented Mar 4, 2020

I'd like to fast-track this once we've explicitly confirmed it as well

@mhart

This comment has been minimized.

Copy link
Contributor

mhart commented Mar 4, 2020

Gotta put my two cents in here and say this still looks like a short-term fix.

So long as tarballs are being created after compilation, issues like this will just keep popping up.

Would love it if the build could just be changed so that binaries are built off the tarball files, so the build only passes if the tarball is good.

@nodejs-github-bot

This comment has been minimized.

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 4, 2020

I can confirm that this fixes the build problem AND that we need the entire folder... I aggressively tried to save us 92k to no avail.

Copy link
Member

MylesBorins left a comment

LGTM

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 4, 2020

Going to go ahead and last this, the failing dgram tests on OSX are known failures

@MylesBorins

This comment has been minimized.

Copy link
Member

MylesBorins commented Mar 4, 2020

Landed in 2130474

@MylesBorins MylesBorins closed this Mar 4, 2020
MylesBorins added a commit that referenced this pull request Mar 4, 2020
PR-URL: #32094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@codebytere codebytere deleted the codebytere:fix-zlib-tarball branch Mar 4, 2020
MylesBorins added a commit that referenced this pull request Mar 4, 2020
PR-URL: #32094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 4, 2020
codebytere added a commit that referenced this pull request Mar 15, 2020
PR-URL: #32094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
codebytere added a commit that referenced this pull request Mar 17, 2020
PR-URL: #32094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
codebytere added a commit that referenced this pull request Mar 30, 2020
PR-URL: #32094
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
You can’t perform that action at this time.