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: add GitHub Action for coverage with --without-intl #37954

Closed
wants to merge 2 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Mar 28, 2021

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.

@nodejs-github-bot nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Mar 28, 2021
@Trott Trott requested a review from bcoe March 28, 2021 05:07
@Trott
Copy link
Member Author

Trott commented Mar 28, 2021

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"

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

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

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

@bcoe
Copy link
Contributor

bcoe commented Mar 30, 2021

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

@Trott
Copy link
Member Author

Trott commented Mar 30, 2021

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

targos commented Mar 30, 2021

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 Trott force-pushed the without-intl-cov branch from f6df6e1 to 76efcc6 Compare March 30, 2021 05:43
@Trott
Copy link
Member Author

Trott commented Mar 30, 2021

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

@nodejs-github-bot

This comment has been minimized.

@targos
Copy link
Member

targos commented Mar 30, 2021

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

#include <unicode/unistr.h>

@targos
Copy link
Member

targos commented Mar 30, 2021

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 Trott force-pushed the without-intl-cov branch from 76efcc6 to 6fa9714 Compare June 28, 2021 20:34
@Trott Trott force-pushed the without-intl-cov branch from 6fa9714 to bcb01c8 Compare July 7, 2021 13:54
@Trott
Copy link
Member Author

Trott commented Jul 8, 2021

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

targos commented Jul 8, 2021

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.

@targos
Copy link
Member

targos commented Jan 16, 2022

Maybe @addaleax can answer our questions about string conversion.

@addaleax
Copy link
Member

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

Trott commented Feb 10, 2022

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

targos commented Feb 12, 2022

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]

@targos
Copy link
Member

targos commented Feb 17, 2023

@Trott
Copy link
Member Author

Trott commented Feb 17, 2023

@targos
Copy link
Member

targos commented Feb 18, 2023

@Trott
Copy link
Member Author

Trott commented Feb 18, 2023

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

Trott commented Feb 18, 2023

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

@Trott 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 github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 18, 2023
@nodejs-github-bot
Copy link
Collaborator

@anonrig
Copy link
Member

anonrig commented Feb 18, 2023

This is so good. Thank you everybody!

@Trott Trott added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 18, 2023
@nodejs-github-bot 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
Copy link
Collaborator

Landed in 3c6547f...4ba860e

nodejs-github-bot pushed a commit that referenced this pull request Feb 18, 2023
PR-URL: #37954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Feb 18, 2023
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@Trott Trott deleted the without-intl-cov branch February 18, 2023 21:53
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
PR-URL: #37954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 18, 2023
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Feb 19, 2023
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
PR-URL: #37954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 20, 2023
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
PR-URL: #37954
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
danielleadams pushed a commit that referenced this pull request Apr 11, 2023
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 <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Yagiz Nizipli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. meta Issues and PRs related to the general management of the project. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants