Skip to content

build: fix zlib tarball generation#32094

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
Closed
codebytere wants to merge 1 commit into from
Closed

build: fix zlib tarball generation #32094

codebytere wants to merge 1 commit into from

Conversation

@codebytere
Copy link
Member

@codebyterecodebytere commented Mar 4, 2020•
edited
Loading

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

@nodejs-github-botnodejs-github-bot added the build Issues and PRs related to build files or the CI. label Mar 4, 2020
@MylesBorins
Copy link
Contributor

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

codebytere commented Mar 4, 2020•
edited
Loading

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

@MylesBorinsMylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 4, 2020
@mhart
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
Copy link
Collaborator

nodejs-github-bot commented Mar 4, 2020

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

@MylesBorins
Copy link
Contributor

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.

MylesBorins
MylesBorins approved these changes Mar 4, 2020
View reviewed changes
Copy link
Contributor

@MylesBorinsMylesBorins left a comment

Choose a reason for hiding this comment

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

LGTM

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 4, 2020

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

@MylesBorins
Copy link
Contributor

MylesBorins commented Mar 4, 2020

Landed in 2130474

MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@codebytere@MylesBorins
build: fix zlib tarball generation
2130474
PR-URL: #32094 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]>
@codebyterecodebytere deleted the fix-zlib-tarball branch March 4, 2020 21:18
MylesBorins pushed a commit that referenced this pull request Mar 4, 2020
@codebytere@MylesBorins
build: fix zlib tarball generation
723aa41
PR-URL: #32094 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Mar 4, 2020
Merged
codebytere added a commit that referenced this pull request Mar 15, 2020
@codebytere
build: fix zlib tarball generation
75453af
PR-URL: #32094 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]>
codebytere added a commit that referenced this pull request Mar 17, 2020
@codebytere
build: fix zlib tarball generation
f5262a4
PR-URL: #32094 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]>
@codebyterecodebytere mentioned this pull request Mar 17, 2020
Merged
codebytere added a commit that referenced this pull request Mar 30, 2020
@codebytere
build: fix zlib tarball generation
038fd25
PR-URL: #32094 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Myles Borins <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@richardlaurichardlaurichardlau approved these changes

+1 more reviewer

@MylesBorinsMylesBorinsMylesBorins approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

buildIssues and PRs related to build files or the CI.fast-trackPRs that do not need to wait for 48 hours to land.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

v13.9.0 tarball seems to be missing deps/zlib/contrib

6 participants

@codebytere@MylesBorins@mhart@nodejs-github-bot@jasnell@richardlau