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

build: detect node_g (debug) for available-node in Makefile #28750

Closed
wants to merge 1 commit into from

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Jul 18, 2019

This lets us run test-ci with debug builds, currently we're using a custom invocation of tools/test.py. Since Node 10, we have --mode=$(BUILDTYPE_LOWER) in Makefile which gets us most of the way there. The doc tests in particular rely on available-node to run and since they are part of test-ci we get a failure because it's not looking for the right executable.

Here's a new version of node-test-commit-linux-containered that runs debug using both build-ci then test-ci rather than the current build-ci and custom tools/test.py invocation and it's green for this branch: https://ci.nodejs.org/job/rvagg-test-commit-linux-containered/11/nodes=ubuntu1604_sharedlibs_debug_x64/

@rvagg rvagg added the build Issues and PRs related to build files or the CI. label Jul 18, 2019
@nodejs-github-bot
Copy link
Collaborator

@rvagg
Copy link
Member Author

rvagg commented Jul 18, 2019

Here's a run that only uses run-ci, via CONFIG_FLAGS=' --debug' make run-ci -j 4: https://ci.nodejs.org/job/rvagg-test-commit-linux-containered/12/nodes=ubuntu1604_sharedlibs_debug_x64

This will partially address nodejs/build#1854, along with #28747 and a new node-test-commit-linux-containered.

@richardlau
Copy link
Member

The change looks okay, but I thought passing --debug to configure built both debug and release?

@Trott
Copy link
Member

Trott commented Jul 18, 2019

The change looks okay, but I thought passing --debug to configure built both debug and release?

I ran make distclean && ./configure --debug && make -j4 and can confirm that it builds both.

@richardlau
Copy link
Member

The change looks okay, but I thought passing --debug to configure built both debug and release?

I ran make distclean && ./configure --debug && make -j4 and can confirm that it builds both.

Which suggests available-node would always find the release node ahead of the debug node_g?

@rvagg
Copy link
Member Author

rvagg commented Jul 19, 2019

yeah, there's something odd about this, it fixes what I want fixed but I can't say why, will investigate further

@Trott
Copy link
Member

Trott commented Jul 22, 2019

yeah, there's something odd about this, it fixes what I want fixed but I can't say why, will investigate further

Any progress on figuring out why this fixes the issue? Anything I or someone else could look at to help?

@rvagg
Copy link
Member Author

rvagg commented Jul 23, 2019

No, haven't had quite enough time to work it out, local debug builds are frustratingly slow!
Current suspicion is that it's a parallelism in make problem, that when it's checking for available-node on the Debug path the Release one isn't available yet, or something like that.

@Trott
Copy link
Member

Trott commented Jul 23, 2019

No, haven't had quite enough time to work it out, local debug builds are frustratingly slow!

I seem to recall that either @targos or @tniessen does frequent Debug builds. Maybe they have some insight or else a super-fast machine on which to troubleshoot.

@addaleax addaleax added the wip Issues and PRs that are still a work in progress. label Nov 30, 2019
@BridgeAR
Copy link
Member

What's the status here?

@rvagg
Copy link
Member Author

rvagg commented Jan 13, 2020

status = low confidence, maybe needs to be revisited at a later date or by someone else

@rvagg rvagg closed this Jan 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. wip Issues and PRs that are still a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants