Reoncile v0.12 with new CI #25541
Reoncile v0.12 with new CI #25541
Conversation
This is a minimal effort to support test output written both to stdout and file in order to get our buildbots understanding test output. Cherry picked from jbergstroem/node@3194073 Original commit metadata follows: PR-URL: nodejs/node#934 Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Snapshots speed up start-up by a few milliseconds but are potentially dangerous because of the fixed hash seed that is used for strings and dictionaries, making collision denial-of-service attacks possible. Release builds on iojs.org have snapshots disabled but source builds did not, until now. The risk for individual source builds is low; the binary gets a random 32 bits hash seed that should be hard to guess by an external attacker. It's when binaries are distributed by, for example, a distro vendor that the fixed hash seed becomes a vulnerability, because then it's possible to target a large group of people at once. People that really need the faster start-up time can use the new --with-snapshot configure flag. Cherry picked from bnoordhuis/io.js@4f68369 Original commit metadata below: PR-URL: nodejs/node#585 Reviewed-By: Bert Belder <bertbelder@gmail.com> Reviewed-By: Johan Bergström <bugs@bergstroem.nu> Reviewed-By: Rod Vagg <rod@vagg.org>
On a few of our installations (namely CentOS), passing 'INFO' resulted in a silent loglevel. Use a logging constant instead. Cherry-picked from nodejs/node@8606793 Original commit metadata follows: Fixes: nodejs/build#104 PR-URL: nodejs/node#1842 Reviewed-By: Rod Vagg <rod@vagg.org>
Since we will run linting before compiling or testing there's no need to run it as part of the ci testing. Cherry-picked from nodejs/node@8d8a26e Original commit metadata follows: PR-URL: nodejs/node#1965 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Revewied-By: Evan Lucas <evanlucas@me.com>
There is now a sepearate linter job taking care of running jslint. See nodejs/node#2004
|
cc @rvagg & @jbergstroem , since I can't mention @nodejs\build. |
|
also cc @misterdjules |
| @@ -306,11 +309,11 @@ $(PKG): release-only | |||
| rm -rf $(PKGDIR) | |||
| rm -rf out/deps out/Release | |||
| $(PYTHON) ./configure --download=all --with-intl=small-icu \ | |||
| --without-snapshot --dest-cpu=ia32 --tag=$(TAG) | |||
There was a problem hiding this comment.
do we know if snapshots are "fixed" in this version of v8? my recollection is that there's a particular version that makes snapshots more safe to ship release builds with
@Fishrock123 do you remember the context of this?
There was a problem hiding this comment.
They are fixed in all io.js versions, but not 0.12, is my understanding. See this maybe: nodejs/node#1663
There was a problem hiding this comment.
Thanks for the heads up. nodejs/node#1631 (comment) doesn't seem to mention a specific v8 version where this was fixed. It probably boils down to calling v8::V8::SetEntropySource() at startup?
In any case, I think we can address the issue of re-enabling v8 snapshots in v0.12 separately (and we should).
In v0.12 we always build without snapshots, so this PR doesn't change that. It is meant to make no-snapshot the default behavior when simply invoking python configure as iojs+any-pr+multi does.
There was a problem hiding this comment.
@orangemocha I'll play the devil's advocate here :)
I'm concerned about the impact of this change for Node.js embedders who might rely on the performance improvement of snapshots being enabled by default. If we change the default build behavior, it might lead a significant performance drop for them. So even though disabling snapshots by default makes it easier for the build process, technically I guess that is a breaking change.
Basically I just want us to be mindful of that use case while considering this change.
There was a problem hiding this comment.
@misterdjules : do you build releases with snapshots or without? I'd argue that the default should be in line with how releases are built.
But if that doesn't work, we could add a configure-for-ci parameter to configure.py.
There was a problem hiding this comment.
@misterdjules : do you build releases with snapshots or without? I'd argue that the default should be in line with how releases are built.
No doubt about that, it's really the impact of changing the behavior of the default build for v0.10 and v0.12 users who build node binaries themselves that I'm wondering about.
But if that doesn't work, we could add a configure-for-ci parameter to configure.py.
I would prefer this approach for v0.10 and v0.12, although I would pick a different option name. We use build-release for vcbuild.bat, maybe we could use that name consistently across platforms. It's not a great name though because both "build" and "release" are ambiguous terms.
|
+1 |
| @@ -127,6 +127,9 @@ test-all-http1: test-build | |||
| test-all-valgrind: test-build | |||
| $(PYTHON) tools/test.py --mode=debug,release --valgrind | |||
|
|
|||
| test-ci: | |||
| $(PYTHON) tools/test.py -ptap --logfile test.tap --mode=release --arch=$(DESTCPU) simple message internet | |||
There was a problem hiding this comment.
Perhaps add an extra space to avoid confusion: -p tap
There was a problem hiding this comment.
@orangemocha Should we pass --flaky-tests=dontcare?
There was a problem hiding this comment.
@jbergstroem : will fix
There was a problem hiding this comment.
@misterdjules : I haven't gotten to that part yet. I was planning to make those changes in a separate PR, but I might as well leave this one open and some commits for the node-accept-pull-request job.
There was a problem hiding this comment.
Sounds good, either works for me.
|
LGTM with minor nit. |
| if "%test%"=="test" set test_args=%test_args% simple message | ||
| if "%test%"=="test-ci" set test_args=%test_args% -p tap --logfile test.tap simple message internet |
There was a problem hiding this comment.
@orangemocha Do we also want to pass --flaky-tests=dontcare here?
|
@orangemocha Added a few comments, but this looks like it's moving in the right direction 👍 |
|
Addressed review feedback in a new PR: #25653. Can you folks please take a look at that one? |
|
Continuing at #25653 |
As part of CI reconciliation, these commits make v0.12 work with https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/