Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

Reoncile v0.12 with new CI #25541

Closed
wants to merge 6 commits into from
Closed

Reoncile v0.12 with new CI #25541

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
2086810
test: support writing test output to file
jbergstroem Jun 9, 2015
67ebb68
build: disable v8 snapshots
bnoordhuis Jan 24, 2015
0a65e72
build: support Jenkins via test-ci
orangemocha Jun 9, 2015
7c8db14
tools: pass constant to logger instead of string
jbergstroem May 30, 2015
77dbf07
build: don't run lint from test-ci
jbergstroem Jun 13, 2015
f6cf940
build: remove lint from test-ci on windows
orangemocha Jun 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps add an extra space to avoid confusion: -p tap

Copy link

Choose a reason for hiding this comment

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

@orangemocha Should we pass --flaky-tests=dontcare?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jbergstroem : will fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

Sounds good, either works for me.


test-release: test-build
$(PYTHON) tools/test.py --mode=release

Expand Down Expand Up @@ -306,11 +309,11 @@ $(PKG): release-only
rm -rf $(PKGDIR)
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

They are fixed in all io.js versions, but not 0.12, is my understanding. See this maybe: nodejs/node#1663

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link

Choose a reason for hiding this comment

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

@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.

rm -rf out/deps out/Release
$(PYTHON) ./configure --download=all --with-intl=small-icu \
--without-snapshot --dest-cpu=ia32 --tag=$(TAG)
--dest-cpu=ia32 --tag=$(TAG)
$(MAKE) install V=$(V) DESTDIR=$(PKGDIR)/32
rm -rf out/deps out/Release
$(PYTHON) ./configure --download=all --with-intl=small-icu \
--without-snapshot --dest-cpu=x64 --tag=$(TAG)
--dest-cpu=x64 --tag=$(TAG)
$(MAKE) install V=$(V) DESTDIR=$(PKGDIR)
SIGN="$(APP_SIGN)" PKGDIR="$(PKGDIR)" bash tools/osx-codesign.sh
lipo $(PKGDIR)/32/usr/local/bin/node \
Expand Down Expand Up @@ -343,7 +346,7 @@ $(BINARYTAR): release-only
rm -rf $(BINARYNAME)
rm -rf out/deps out/Release
$(PYTHON) ./configure --prefix=/ --download=all --with-intl=small-icu \
--without-snapshot --dest-cpu=$(DESTCPU) --tag=$(TAG) $(CONFIG_FLAGS)
--dest-cpu=$(DESTCPU) --tag=$(TAG) $(CONFIG_FLAGS)
$(MAKE) install DESTDIR=$(BINARYNAME) V=$(V) PORTABLE=1
cp README.md $(BINARYNAME)
cp LICENSE $(BINARYNAME)
Expand All @@ -356,7 +359,7 @@ binary: $(BINARYTAR)

$(PKGSRC): release-only
rm -rf dist out
$(PYTHON) configure --prefix=/ --without-snapshot --download=all \
$(PYTHON) configure --prefix=/ --download=all \
--with-intl=small-icu --dest-cpu=$(DESTCPU) --tag=$(TAG) \
$(CONFIG_FLAGS)
$(MAKE) install DESTDIR=dist
Expand Down
1 change: 0 additions & 1 deletion android-configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,5 @@ export CXX=arm-linux-androideabi-g++
export LINK=arm-linux-androideabi-g++

./configure \
--without-snapshot \
--dest-cpu=arm \
--dest-os=android
21 changes: 16 additions & 5 deletions configure
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,11 +292,16 @@ parser.add_option('--without-perfctr',
dest='without_perfctr',
help='build without performance counters')

parser.add_option('--with-snapshot',
action='store_true',
dest='with_snapshot',
help=optparse.SUPPRESS_HELP)
Copy link

Choose a reason for hiding this comment

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

optparse.SUPPRESS_HELP means that --with-snapshot doesn't show when running configure --help, I think it would be helpful to add some help similar to the original help that was available for --without-snapshot.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will fix


# Dummy option for backwards compatibility.
parser.add_option('--without-snapshot',
action='store_true',
dest='without_snapshot',
help='build without snapshotting V8 libraries. You might want to set'
' this for cross-compiling. [Default: False]')
dest='unused_without_snapshot',
help=optparse.SUPPRESS_HELP)

parser.add_option('--without-ssl',
action='store_true',
Expand All @@ -323,6 +328,12 @@ parser.add_option('--xcode',
# set up auto-download list
auto_downloads = nodedownload.parse(options.download_list)


def warn(msg):
prefix = '\033[1m\033[91mWARNING\033[0m' if os.isatty(1) else 'WARNING'
print('%s: %s' % (prefix, msg))


def b(value):
"""Returns the string 'true' if value is truthy, 'false' otherwise."""
if value:
Expand Down Expand Up @@ -515,7 +526,7 @@ def configure_node(o):
o['variables']['host_arch'] = host_arch
o['variables']['target_arch'] = target_arch

if target_arch != host_arch and not options.without_snapshot:
if target_arch != host_arch and options.with_snapshot:
o['variables']['want_separate_host_toolset'] = 1
else:
o['variables']['want_separate_host_toolset'] = 0
Expand Down Expand Up @@ -651,7 +662,7 @@ def configure_v8(o):
o['variables']['v8_no_strict_aliasing'] = 1 # Work around compiler bugs.
o['variables']['v8_optimized_debug'] = 0 # Compile with -O0 in debug builds.
o['variables']['v8_random_seed'] = 0 # Use a random seed for hash tables.
o['variables']['v8_use_snapshot'] = b(not options.without_snapshot)
o['variables']['v8_use_snapshot'] = b(options.with_snapshot)

# assume shared_v8 if one of these is set?
if options.shared_v8_libpath:
Expand Down
2 changes: 1 addition & 1 deletion node.gyp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
'variables': {
'v8_use_snapshot%': 'true',
'v8_use_snapshot%': 'false',
'node_use_dtrace%': 'false',
'node_use_etw%': 'false',
'node_use_perfctr%': 'false',
Expand Down
28 changes: 20 additions & 8 deletions tools/test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@


import imp
import logging
import optparse
import os
import platform
Expand All @@ -45,6 +46,8 @@
from datetime import datetime
from Queue import Queue, Empty

logger = logging.getLogger('testrunner')

VERBOSE = False


Expand Down Expand Up @@ -225,7 +228,7 @@ def HasRun(self, output):
class TapProgressIndicator(SimpleProgressIndicator):

def Starting(self):
print '1..%i' % len(self.cases)
logger.info('1..%i' % len(self.cases))
self._done = 0

def AboutToRun(self, case):
Expand All @@ -238,26 +241,26 @@ def HasRun(self, output):
status_line = 'not ok %i - %s' % (self._done, command)
if FLAKY in output.test.outcomes and self.flaky_tests_mode == "dontcare":
status_line = status_line + " # TODO : Fix flaky test"
print status_line
logger.info(status_line)
for l in output.output.stderr.splitlines():
print '#' + l
logger.info('#' + l)
for l in output.output.stdout.splitlines():
print '#' + l
logger.info('#' + l)
else:
status_line = 'ok %i - %s' % (self._done, command)
if FLAKY in output.test.outcomes:
status_line = status_line + " # TODO : Fix flaky test"
print status_line
logger.info(status_line)

duration = output.test.duration

# total_seconds() was added in 2.7
total_seconds = (duration.microseconds +
(duration.seconds + duration.days * 24 * 3600) * 10**6) / 10**6

print ' ---'
print ' duration_ms: %d.%d' % (total_seconds, duration.microseconds / 1000)
print ' ...'
logger.info(' ---')
logger.info(' duration_ms: %d.%d' % (total_seconds, duration.microseconds / 1000))
logger.info(' ...')

def Done(self):
pass
Expand Down Expand Up @@ -1192,6 +1195,8 @@ def BuildOptions():
default='release')
result.add_option("-v", "--verbose", help="Verbose output",
default=False, action="store_true")
result.add_option('--logfile', dest='logfile',
help='write test output to file. NOTE: this only applies the tap progress indicator')
result.add_option("-S", dest="scons_flags", help="Flag to pass through to scons",
default=[], action="append")
result.add_option("-p", "--progress",
Expand Down Expand Up @@ -1368,6 +1373,13 @@ def Main():
parser.print_help()
return 1

ch = logging.StreamHandler(sys.stdout)
logger.addHandler(ch)
logger.setLevel(logging.INFO)
if options.logfile:
fh = logging.FileHandler(options.logfile)
logger.addHandler(fh)

workspace = abspath(join(dirname(sys.argv[0]), '..'))
suites = GetSuites(join(workspace, 'test'))
repositories = [TestRepository(join(workspace, 'test', name)) for name in suites]
Expand Down
22 changes: 13 additions & 9 deletions vcbuild.bat
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ set msiplatform=x86
set target=Build
set target_arch=ia32
set debug_arg=
set nosnapshot_arg=
set snapshot_arg=
set noprojgen=
set nobuild=
set nosign=
set nosnapshot=
set snapshot=
set test=
set test_args=
set msi=
Expand Down Expand Up @@ -50,7 +50,7 @@ if /i "%1"=="x64" set target_arch=x64&goto arg-ok
if /i "%1"=="noprojgen" set noprojgen=1&goto arg-ok
if /i "%1"=="nobuild" set nobuild=1&goto arg-ok
if /i "%1"=="nosign" set nosign=1&goto arg-ok
if /i "%1"=="nosnapshot" set nosnapshot=1&goto arg-ok
if /i "%1"=="snapshot" set snapshot=1&goto arg-ok
if /i "%1"=="noetw" set noetw=1&goto arg-ok
if /i "%1"=="noperfctr" set noperfctr=1&goto arg-ok
if /i "%1"=="licensertf" set licensertf=1&goto arg-ok
Expand All @@ -61,7 +61,8 @@ if /i "%1"=="test-simple" set test=test-simple&goto arg-ok
if /i "%1"=="test-message" set test=test-message&goto arg-ok
if /i "%1"=="test-gc" set test=test-gc&set buildnodeweak=1&goto arg-ok
if /i "%1"=="test-all" set test=test-all&set buildnodeweak=1&goto arg-ok
if /i "%1"=="test" set test=test&goto arg-ok
if /i "%1"=="test-ci" set test=test-ci&goto arg-ok
if /i "%1"=="test" set test=test&set jslint=1&goto arg-ok
@rem Include small-icu support with MSI installer
if /i "%1"=="msi" set msi=1&set licensertf=1&set download_arg="--download=all"&set i18n_arg=small-icu&goto arg-ok
if /i "%1"=="upload" set upload=1&goto arg-ok
Expand All @@ -81,10 +82,9 @@ goto next-arg

:args-done
if defined upload goto upload
if defined jslint goto jslint

if defined build_release (
set nosnapshot=1
set snapshot=
set config=Release
set msi=1
set licensertf=1
Expand All @@ -94,7 +94,7 @@ if defined build_release (

if "%config%"=="Debug" set debug_arg=--debug
if "%target_arch%"=="x64" set msiplatform=x64
if defined nosnapshot set nosnapshot_arg=--without-snapshot
if defined snapshot set snapshot_arg=--with-snapshot
if defined noetw set noetw_arg=--without-etw& set noetw_msi_arg=/p:NoETW=1
if defined noperfctr set noperfctr_arg=--without-perfctr& set noperfctr_msi_arg=/p:NoPerfCtr=1

Expand Down Expand Up @@ -151,7 +151,7 @@ goto exit
if defined noprojgen goto msbuild

@rem Generate the VS project.
python configure %download_arg% %i18n_arg% %debug_arg% %nosnapshot_arg% %noetw_arg% %noperfctr_arg% --dest-cpu=%target_arch% --tag=%TAG%
python configure %download_arg% %i18n_arg% %debug_arg% %snapshot_arg% %noetw_arg% %noperfctr_arg% --dest-cpu=%target_arch% --tag=%TAG%
if errorlevel 1 goto create-msvs-files-failed
if not exist node.sln goto create-msvs-files-failed
echo Project files generated.
Expand Down Expand Up @@ -202,7 +202,11 @@ if "%test%"=="" goto exit
if "%config%"=="Debug" set test_args=--mode=debug
if "%config%"=="Release" set test_args=--mode=release

set test_args=%test_args% --arch=%target_arch%


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

Choose a reason for hiding this comment

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

@orangemocha Do we also want to pass --flaky-tests=dontcare here?

if "%test%"=="test-internet" set test_args=%test_args% internet
if "%test%"=="test-pummel" set test_args=%test_args% pummel
if "%test%"=="test-simple" set test_args=%test_args% simple
Expand All @@ -224,7 +228,7 @@ goto exit
:run-tests
echo running 'python tools/test.py %test_args%'
python tools/test.py %test_args%
if "%test%"=="test" goto jslint
if defined jslint goto jslint
goto exit

:create-msvs-files-failed
Expand Down