build: add GitHub Action for coverage with --without-intl #37954

Trott · 2021-03-28T05:07:07Z

There are parts of the code base that require a build without intl to be
covered. So add a coverage job to build --without-intl.

Trott · 2021-03-28T05:07:50Z

Here's the diff between the existing Linux coverage job and this one:

1c1
< name: coverage-linux-without-intl
---
> name: coverage-linux
24c24
<   coverage-linux-without-intl:
---
>   coverage-linux:
38c38
<         run: make build-ci -j2 V=1 CONFIG_FLAGS="--error-on-warn --without-intl --coverage"
---
>         run: make build-ci -j2 V=1 CONFIG_FLAGS="--error-on-warn --coverage"

richardlau

LGTM, although since the diff to the existing Linux coverage job is so small maybe consider matrixing the existing workflow?

bcoe · 2021-03-30T04:53:02Z

@Trott it looks like the issue is that the inspector is not enabled does --without-intl turn off the inspector?

Trott · 2021-03-30T05:05:06Z

@Trott it looks like the issue is that the inspector is not enabled does --without-intl turn off the inspector?

Yes, it indeed disables the inspector.

node/configure.py

Lines 545 to 549 in 7823759

    
           intl_optgroup.add_argument('--without-intl', 
        
               action='store_const', 
        
               dest='with_intl', 
        
               const='none', 
        
               help='Disable Intl, same as --with-intl=none (disables inspector)')

targos · 2021-03-30T05:32:12Z

Can you try to remove this line?

node/configure.py

Line 1797 in 7823759

options.with_intl in (None, 'none') or

Maybe it is now possible to enable the inspector without intl.

Trott · 2021-03-30T05:43:32Z

Can you try to remove this line?

node/configure.py

Line 1797 in 7823759

options.with_intl in (None, 'none') or

Maybe it is now possible to enable the inspector without intl.

Done and force-pushed. Now we wait... 🤞

targos · 2021-03-30T05:46:43Z

Well I just tried locally and it failed in our source:

node/src/inspector/main_thread_interface.cc

Line 8 in 7823759

#include <unicode/unistr.h>

targos · 2021-03-30T05:49:02Z

It seems we only include ICU to convert UTF-8 to UTF-16 (at least in this file). Maybe V8 has an API for that?

Trott · 2021-07-08T05:05:19Z

It seems we only include ICU to convert UTF-8 to UTF-16 (at least in this file). Maybe V8 has an API for that?

My C++ is not good, but does it seem do-able using v8::String::NewFromUtf8() along with v8::String::Encoding set to TWO_BYTE_ENCODING or something like that?

targos · 2021-07-08T09:32:06Z

There's also https://github.com/nodejs/node/blob/master/src/inspector/node_string.cc

I'm not good enough with C++ to tell whether it can be rewritten using V8 APIs instead of ICU.

.github/workflows/coverage-linux-without-intl.yml

targos · 2022-01-16T14:42:43Z

Maybe @addaleax can answer our questions about string conversion.

addaleax · 2022-01-17T13:09:18Z

So, if the issue is converting from UTF-8 to UTF-16, then v8::String::NewFromUtf8(...)->Write(...) will do the trick, yeah. It requires being run on a thread with an active V8 Isolate, I’m not sure if that’s the case here or not.

There’s also built-in C++ standard utilities in the codecvt header, which unfortunately are being deprecated, but I guess for now those would present a good alternative.

And finally, there’s always the option of using some external code for this job. I don’t think that would be the end of the world in any way.

Trott · 2022-02-10T03:23:10Z

I think I got it working in src/inspector/main_thread_interface.cc but I still have to do it for src/inspector/node_string.cc. But we may be close here.

targos · 2022-02-12T11:55:25Z

FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
 1: 0x5562e2df6584 node::Abort() [out/Release/cctest]
 2: 0x5562e27df4e2 node::FatalError(char const*, char const*) [out/Release/cctest]
 3: 0x5562e3366fce v8::Utils::ReportApiFailure(char const*, char const*) [out/Release/cctest]
 4: 0x5562e344ada6 v8::internal::HandleScope::Extend(v8::internal::Isolate*) [out/Release/cctest]
 5: 0x5562e345010c v8::internal::FactoryBase<v8::internal::Factory>::NewRawOneByteString(int, v8::internal::AllocationType) [out/Release/cctest]
 6: 0x5562e345840d v8::internal::Factory::NewStringFromUtf8(v8::base::Vector<char const> const&, v8::internal::AllocationType) [out/Release/cctest]
 7: 0x5562e337b357 v8::String::NewFromUtf8(v8::Isolate*, char const*, v8::NewStringType, int) [out/Release/cctest]
 8: 0x5562e2f387fb node::inspector::Utf8ToStringView(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) [out/Release/cctest]

src/inspector/main_thread_interface.cc

targos · 2023-02-17T16:40:51Z

CI to watch: https://github.com/nodejs/node/actions/runs/4205696552/jobs/7298166015

Trott · 2023-02-17T23:33:17Z

CI to watch: https://github.com/nodejs/node/actions/runs/4205696552/jobs/7298166015

🎉 It's green!

targos · 2023-02-18T09:08:48Z

I think this is the codecov result: https://app.codecov.io/gh/nodejs/node/commit/5c0f877885136cef6a0f12894688c6fc4e7ad2c2

Trott · 2023-02-18T15:27:57Z

I think this is the codecov result: https://app.codecov.io/gh/nodejs/node/commit/5c0f877885136cef6a0f12894688c6fc4e7ad2c2

And here's the if statement that motivated this PR in the first place. https://app.codecov.io/gh/nodejs/node/commit/5c0f877885136cef6a0f12894688c6fc4e7ad2c2/blob/lib/internal/util/inspect.js#L2298
The else block is now covered (except for one line, but that can be a test we add in another PR). 🎉

Trott · 2023-02-18T15:29:05Z

Because this changes configure.py, a full CI seems warranted to check for unintended consequences (especially in the non-Intl build on Jenkins.

nodejs-github-bot · 2023-02-18T15:37:40Z

CI: https://ci.nodejs.org/job/node-test-pull-request/49691/

anonrig · 2023-02-18T15:53:17Z

This is so good. Thank you everybody!

nodejs-github-bot · 2023-02-18T19:40:36Z

Landed in 3c6547f...4ba860e

PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

There are parts of the code base that require a build without intl to be covered. So add a coverage job to build --without-intl. PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

There are parts of the code base that require a build without intl to be covered. So add a coverage job to build --without-intl. PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

There are parts of the code base that require a build without intl to be covered. So add a coverage job to build --without-intl. PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

There are parts of the code base that require a build without intl to be covered. So add a coverage job to build --without-intl. PR-URL: #37954 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Mar 28, 2021

Trott requested a review from bcoe March 28, 2021 05:07

targos approved these changes Mar 28, 2021

View reviewed changes

richardlau approved these changes Mar 28, 2021

View reviewed changes

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

Trott force-pushed the without-intl-cov branch from f6df6e1 to 76efcc6 Compare March 30, 2021 05:43

This comment has been minimized.

Sign in to view

Trott force-pushed the without-intl-cov branch from 76efcc6 to 6fa9714 Compare June 28, 2021 20:34

Trott force-pushed the without-intl-cov branch from 6fa9714 to bcb01c8 Compare July 7, 2021 13:54

jasnell approved these changes Jul 8, 2021

View reviewed changes

richardlau mentioned this pull request Jul 14, 2021

CI job request: daily coverage job without Intl nodejs/build#2596

Closed

Trott force-pushed the without-intl-cov branch from bcb01c8 to 5df1334 Compare January 14, 2022 22:31

Mesteery reviewed Jan 15, 2022

View reviewed changes

.github/workflows/coverage-linux-without-intl.yml Outdated Show resolved Hide resolved

Trott force-pushed the without-intl-cov branch from 3545058 to f313d1c Compare January 16, 2022 00:04

Trott force-pushed the without-intl-cov branch from f313d1c to f6a1e5a Compare February 10, 2022 02:44

Trott force-pushed the without-intl-cov branch from f6a1e5a to 47fad38 Compare February 10, 2022 03:32

targos reviewed Feb 12, 2022

View reviewed changes

src/inspector/main_thread_interface.cc Outdated Show resolved Hide resolved

targos force-pushed the without-intl-cov branch from d68418b to 5c0f877 Compare February 17, 2023 16:36

anonrig approved these changes Feb 18, 2023

View reviewed changes

targos approved these changes Feb 18, 2023

View reviewed changes

Trott added needs-ci PRs that need a full CI run. request-ci Add this label to start a Jenkins CI on a PR. and removed help wanted Issues that need assistance from volunteers or PRs that need help to proceed. labels Feb 18, 2023

Trott added the commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. label Feb 18, 2023

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023

Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023

nodejs-github-bot closed this Feb 18, 2023

Trott deleted the without-intl-cov branch February 18, 2023 21:53

MylesBorins mentioned this pull request Feb 19, 2023

v19.7.0 proposal #46725

Merged

danielleadams mentioned this pull request Apr 11, 2023

v18.16.0 release proposal #47502

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: add GitHub Action for coverage with --without-intl #37954

build: add GitHub Action for coverage with --without-intl #37954

Trott commented Mar 28, 2021

Trott commented Mar 28, 2021

richardlau left a comment

bcoe commented Mar 30, 2021

Trott commented Mar 30, 2021

targos commented Mar 30, 2021

Trott commented Mar 30, 2021

This comment has been minimized.

targos commented Mar 30, 2021

targos commented Mar 30, 2021

Trott commented Jul 8, 2021

targos commented Jul 8, 2021

targos commented Jan 16, 2022

addaleax commented Jan 17, 2022

Trott commented Feb 10, 2022

targos commented Feb 12, 2022

targos commented Feb 17, 2023

Trott commented Feb 17, 2023

targos commented Feb 18, 2023

Trott commented Feb 18, 2023 •

edited

Loading

Trott commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

anonrig commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

build: add GitHub Action for coverage with --without-intl #37954

build: add GitHub Action for coverage with --without-intl #37954

Conversation

Trott commented Mar 28, 2021

Trott commented Mar 28, 2021

richardlau left a comment

Choose a reason for hiding this comment

bcoe commented Mar 30, 2021

Trott commented Mar 30, 2021

targos commented Mar 30, 2021

Trott commented Mar 30, 2021

This comment has been minimized.

targos commented Mar 30, 2021

targos commented Mar 30, 2021

Trott commented Jul 8, 2021

targos commented Jul 8, 2021

targos commented Jan 16, 2022

addaleax commented Jan 17, 2022

Trott commented Feb 10, 2022

targos commented Feb 12, 2022

targos commented Feb 17, 2023

Trott commented Feb 17, 2023

targos commented Feb 18, 2023

Trott commented Feb 18, 2023 • edited Loading

Trott commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

anonrig commented Feb 18, 2023

nodejs-github-bot commented Feb 18, 2023

Trott commented Feb 18, 2023 •

edited

Loading