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

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

Closed
mhart opened this issue Feb 18, 2020 · 17 comments
Closed

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

mhart opened this issue Feb 18, 2020 · 17 comments
Assignees

Comments

@mhart
Copy link
Contributor

mhart commented Feb 18, 2020

curl -sSL https://nodejs.org/download/release/v13.9.0/node-v13.9.0.tar.xz | \
  tar -tJ | grep "deps/zlib/contrib"

(no results)

This results in build failures, eg:

  CC(target) /home/node/node-v13.9.0/out/Release/obj.target/zlib/deps/zlib/gzclose.o
../deps/zlib/deflate.c:54:49: fatal error: contrib/optimizations/insert_string.h: No such file or directory
 #include "contrib/optimizations/insert_string.h"
@mhart
Copy link
Contributor Author

mhart commented Feb 18, 2020

I feel this has cropped up before with the tarball missing things (typically new directories) – there's some sort of whitelist somewhere from memory?

@mhart
Copy link
Contributor Author

mhart commented Feb 18, 2020

And there was talk at the time of ensuring that a source tarball build succeeded before pushing a release out – not sure where that got to

@codebytere
Copy link
Member

@mhart hmm ty for the catch - at first blush i'm pretty sure this happened when we switched to Chromium's zlib impl in #31201

cc @mscdex, who may know more?

@mhart
Copy link
Contributor Author

mhart commented Feb 18, 2020

Ah yeah, happened in v12.11.0 too:

#29712

nodejs/build#1931

@richardlau richardlau transferred this issue from nodejs/build Feb 18, 2020
@richardlau
Copy link
Member

The source tarball is built by stripping out things from the source tree:

node/Makefile

Lines 1018 to 1054 in 0c3c0e7

$(TARBALL): release-only $(NODE_EXE) doc
git checkout-index -a -f --prefix=$(TARNAME)/
mkdir -p $(TARNAME)/doc/api
cp doc/node.1 $(TARNAME)/doc/node.1
cp -r out/doc/api/* $(TARNAME)/doc/api/
$(RM) -r $(TARNAME)/.editorconfig
$(RM) -r $(TARNAME)/.git*
$(RM) -r $(TARNAME)/.mailmap
$(RM) -r $(TARNAME)/deps/openssl/openssl/demos
$(RM) -r $(TARNAME)/deps/openssl/openssl/doc
$(RM) -r $(TARNAME)/deps/openssl/openssl/test
$(RM) -r $(TARNAME)/deps/uv/docs
$(RM) -r $(TARNAME)/deps/uv/samples
$(RM) -r $(TARNAME)/deps/uv/test
$(RM) -r $(TARNAME)/deps/v8/samples
$(RM) -r $(TARNAME)/deps/v8/tools/profviz
$(RM) -r $(TARNAME)/deps/v8/tools/run-tests.py
$(RM) -r $(TARNAME)/deps/zlib/contrib # too big, unused
$(RM) -r $(TARNAME)/doc/images # too big
$(RM) -r $(TARNAME)/test*.tap
$(RM) -r $(TARNAME)/tools/cpplint.py
$(RM) -r $(TARNAME)/tools/eslint-rules
$(RM) -r $(TARNAME)/tools/license-builder.sh
$(RM) -r $(TARNAME)/tools/node_modules
$(RM) -r $(TARNAME)/tools/osx-*
$(RM) -r $(TARNAME)/tools/osx-pkg.pmdoc
find $(TARNAME)/deps/v8/test/* -type d ! -regex '.*/test/torque$$' | xargs $(RM) -r
find $(TARNAME)/deps/v8/test -type f ! -regex '.*/test/torque/.*' | xargs $(RM)
find $(TARNAME)/ -name ".eslint*" -maxdepth 2 | xargs $(RM)
find $(TARNAME)/ -type l | xargs $(RM) # annoying on windows
tar -cf $(TARNAME).tar $(TARNAME)
$(RM) -r $(TARNAME)
gzip -c -f -9 $(TARNAME).tar > $(TARNAME).tar.gz
ifeq ($(XZ), 1)
xz -c -f -$(XZ_COMPRESSION) $(TARNAME).tar > $(TARNAME).tar.xz
endif
$(RM) $(TARNAME).tar

Up until now we exclude everything in deps/zlib/contrib:

node/Makefile

Line 1035 in 0c3c0e7

$(RM) -r $(TARNAME)/deps/zlib/contrib # too big, unused

but #31201 has changed things. Is it just contrib/optimizations that we need to add?

@mscdex
Copy link
Contributor

mscdex commented Feb 18, 2020

Is it just contrib/optimizations that we need to add?

Probably.

@mhart
Copy link
Contributor Author

mhart commented Feb 19, 2020

Could the Makefile be modified so that all binaries are built off the result of the tarball modifications? That would catch issues like this before releases go out.

@mistydemeo
Copy link
Contributor

Looks like this issue made it into the 13.10.0 tarball, too.

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

@mhart
Copy link
Contributor Author

mhart commented Mar 4, 2020

Ugh, that's... pretty poor. Any comments on my suggestion?

@mistydemeo
Copy link
Contributor

Is there a resolution to this, or any chance for a 13.10.1 with a fixed tarball? I work on Homebrew; we weren't able to package 13.9.0 because we couldn't build from the release tarball, and we're not going to be able to package 13.10.0 either.

@csvn
Copy link

csvn commented Mar 4, 2020

The official docker node image seems to be blocked by the same issue too. It would be great if the latest node version was available to use from containers.

@MylesBorins
Copy link
Member

Hey All, I'm going to dig into this right now and see if we can get a quick 13.10.1 out to fix the problem.

@MylesBorins
Copy link
Member

FYI a fix landed on master and there is a proposal for a release

#32099

@MylesBorins
Copy link
Member

Hey all. Have an rc built with the potential fix PTAL

https://nodejs.org/download/rc/v13.10.1-rc.0/

@mistydemeo
Copy link
Contributor

Is there a generated source tarball for the potential fix? I think that's the most important thing to test. I can give it a go from Homebrew's perspective if there is.

@MylesBorins
Copy link
Member

Sorry I took so long to get back to you

https://nodejs.org/download/rc/v13.10.1-rc.0/node-v13.10.1-rc.0.tar.gz

I've pulled it down and tested myself on osx 10.14.6

@MylesBorins
Copy link
Member

This is fixed in v13.10.1

Thanks for the patience everyone

branchseer added a commit to branchseer/libnode that referenced this issue Mar 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants