Changes needed to Makefile for debug build & test

[rvagg]

I'm making this here and not in nodejs/node cause I'll lose track of it and the people most likely to work on it are probably going to be in here anyway.

• run-ci isn't usable, it assumes release builds
• build-ci is usable with CONFIG_FLAGS="--debug"
• test-ci isn't usable, you have to call tools/test.py manually.
• The addon and addon-api (and I think doctool but I haven't looked in detail) tests don't work, there's no way to get down into the node-gyp call and pass --debug

FYI these are relevant parts of the Debug build Jenkins script I've been playing with that works so far. It'd be nice to be able to call run-ci with some modifier, like DEBUG or BUILD_TYPE=Debug or even a new target run-debug-ci.

PYTHON=python \
 NODE_TEST_DIR=${HOME}/node-tmp \
 FLAKY_TESTS=$FLAKY_TESTS_MODE \
 CONFIG_FLAGS="--debug" \
 make build-ci -j $JOBS

python tools/test.py -j $JOBS -p tap --logfile test.tap \
  --mode=debug --flaky-tests=$FLAKY_TESTS_MODE \
  async-hooks default addons addons-napi \
  doctool known_issues

Fails on 3 core tests, I've noted them here: nodejs/node#17016, nodejs/node#17017, nodejs/node#17018

See https://ci.nodejs.org/job/node-test-commit-linux-linked/nodes=ubuntu1604_sharedlibs_debug_x64/43/ for a current run with this config and associated failures

@nodejs/testing: I'm going to remove addons addons-napi doctool from the test run and add the 3 failing core tests manually to the flaky list from within jenkins so we can at least get to "flaky" status for these debug builds and then try them out attached to node-test-commit.

[rvagg]

This is in Jenkins:

# see https://github.com/nodejs/node/issues/17016
sed -i 's/\[\$system==linux\]/[$system==linux]\ntest-error-reporting : PASS, FLAKY/g' test/parallel/parallel.status
# see https://github.com/nodejs/node/issues/17017
sed -i 's/\[\$system==linux\]/[$system==linux]\ntest-inspector-async-stack-traces-promise-then : PASS, FLAKY/g' test/sequential/sequential.status
# see https://github.com/nodejs/node/issues/17018
sed -i 's/\[\$system==linux\]/[$system==linux]\ntest-inspector-contexts : PASS, FLAKY/g' test/sequential/sequential.status

For this kind of result: https://ci.nodejs.org/job/node-test-commit-linux-linked/48/nodes=ubuntu1604_sharedlibs_debug_x64/

I've now linked this up to node-test-commit, it's associated with node-test-commit-linux-linked, which, in this case is a misnomer, but "linked" is meant to be for dynamic linking tests for openssl, zlib, c-ares, etc. OpenSSL 1.1.0 is in there already, as is OpenSSL-FIPS.

[juggernaut451]

@rvagg is this still open?

[rvagg]

@juggernaut451 yes, I believe so, are you interested in tackling it?

[juggernaut451]

@rvagg yes I am totally interested. can you mentor me on this?

[rvagg]

@juggernaut451 sure, how much experience do you have with Makefiles? Your challenge, should you choose to accept, is to make make run-ci build and test debug builds.

There is a variable near the top, BUILDTYPE that we should be able to use. It has a variation, BUILDTYPE_LOWER, that's useful in certain places. I think it'd be ideal if we could just make run-ci BUILDTYPE=Debug and have it work. Right now the default is make run-ci BUILDTYPE=Release because that's the default for that variable.

You can see in test-ci that --mode=release is hardcoded. That's easy, just make it BUILDTYPE_LOWER. It also does a ps and looks for Release/node which is also wrong. I think $(BUILDTYPE)/$(NODE_EXE) will do for that.

For build-ci I think it's going to need some logic to insert --debug if BUILDTYPE is "Debug". There'll be a few ways to achieve that, including putting it in CONFIG_FLAGS if you like but you'll also have to make sure it doesn't overwrite any other CONFIG_FLAGS the user might pass in, just append to it.

Lastly, run-ci just needs to delegate. Since build-ci is a dependency then that should be taken care of as BUILDTYPE will pass on to it automatically. For the call to test-ci I think it might need to become $(MAKE) test-ci BUILDTYPE=$(BUILDTYPE) but perhaps you could test if that's necessary.

There's also a bunch of other little stuff you might want to consider, like do the addons get built as debug during all of this? It's going to take a bit of tinkering.

Be aware when you're playing with this that a debug build does a release & a debug build, thanks to this bit:

ifeq ($(BUILDTYPE),Release)
all: out/Makefile $(NODE_EXE) ## Default target, builds node in out/Release/node.
else
all: out/Makefile $(NODE_EXE) $(NODE_G_EXE)
endif

I don't know why we do that, it's been doing it for longer than I've been around so I dare not change it. Just be aware that a build will take more than twice as long since it does two builds. If you don't already have it, I'd recommend you install ccache to speed this up across multiple runs.

[juggernaut451]

working on it

[juggernaut451]

@rvagg where could I find run-ci?

[gibfahn]

@juggernaut451 :

https://github.com/nodejs/node/blob/1d2ab79f2ce0057df1c0f0fe5eef15c143ad9942/Makefile#L474

[juggernaut451]

@rvagg test/parallel/test-process-config.js it's failing because of process.config is having default_configuration: 'Release' and config as default_configuration: 'Debug'. Can you please help me as where to make changes so that default_configuration: 'Debug'in process.config

[juggernaut451]

above issue has been resolved by ./configure --debug.
Can someone tell me how are addon build as the addon test are failing with following error
Cannot find module './build/Debug/addon'

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.