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

testing with small icu #2998

Closed
Trott opened this issue Jul 14, 2022 · 14 comments
Closed

testing with small icu #2998

Trott opened this issue Jul 14, 2022 · 14 comments

Comments

@Trott
Copy link
Member

Trott commented Jul 14, 2022

It was noted (by @bnoordhuis) that test/parallel/test-icu-data-dir.js only runs with small-icu builds and we don't seem to have those in CI anymore. It used to be tested in CI on the Raspberry Pi devices, but they don't run on the main branch anymore. (They seem to run against v12.x staging daily. Since we don't support v12.x anymore, I suppose we could/should remove that nightly job?)

Should we add something to the linux-containered group that compiles with small ICU so we can continue to exercise that test?

@richardlau
Copy link
Member

(They seem to run against v12.x staging daily. Since we don't support v12.x anymore, I suppose we could/should remove that nightly job?)

Just on this point -- I've kept the job because we don't have a v14.x or v16.x nightly job and the existing v12.x job gives me some confidence that the CI machines still work (for example we no longer run builds on the CentOS 7/RHEL 7 machines for Node.js 18 and later).

@mhdawson
Copy link
Member

Working on adding small-icu to the contained tests.

  1. added tags to all of the contairned machines - ubuntu1804_sharedlibs_smallicu_x64
  2. Testing out here to make sure tests pass - https://ci.nodejs.org/view/All/job/node-test-commit-linux-small-icu/nodes=ubuntu1804_sharedlibs_smallicu_x64/5/

Once tests pass will add to regular containered job.

@mhdawson
Copy link
Member

@richardlau
Copy link
Member

richardlau commented Jan 18, 2023

@mhdawson
Copy link
Member

Looks like it passes for 16, will add so that it only runs on 16 and later for now.

@mhdawson
Copy link
Member

@richardlau I assume that's because small-icu is broken on 14 and not the job itself ?

We not only have a different version of ICU in 14 but also looks like some of the tools are slightly different versions?

The script that is failing though https://github.com/nodejs/node/tree/main/tools/icu)/icutrim.py seems to be the same version though as what we have in main

failure reported

07:47:42   File "icutrim.py", line 339, in <module>
07:47:42     removeList(1)
07:47:42   File "icutrim.py", line 319, in removeList
07:47:42     pat = re.compile(bytes(r"^Item ([^ ]+) depends on missing item ([^ ]+).*", 'utf-8'))
07:47:42 TypeError: str() takes at most 1 argument (2 given)
07:47:42 tools/icu/icudata.target.mk:13: recipe for target '/home/iojs/build/workspace/node-test-commit-linux-small-icu/out/Release/obj/gen/icutmp/icudt70l.dat' failed
07:47:42 make[2]: *** [/home/iojs/build/workspace/node-test-commit-linux-small-icu/out/Release/obj/gen/icutmp/icudt70l.dat] Error 1
07:47:42 make[2]: *** Waiting for unfinished jobs....

@richardlau
Copy link
Member

@mhdawson I would guess it's a Python 2 vs Python 3 issue. Node.js 14 builds with Python 2 by default.

@mhdawson
Copy link
Member

mhdawson commented Jan 18, 2023

@richardlau thanks for the suggestion of Python 2 versus Python 3, I was wondering if that might be the case as well.

@mhdawson
Copy link
Member

Built locally to reproduce but all tests passed, likely because my system used Python3. I think that points in the direction of it being a Python2 problem as well.

@mhdawson
Copy link
Member

Yup switching to ubuntu with python 2 and I get the same error as in the job.

@mhdawson
Copy link
Member

Working on a fix

mhdawson added a commit to mhdawson/io.js that referenced this issue Jan 18, 2023
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
@mhdawson
Copy link
Member

PR with fix - nodejs/node#46263.

@richardlau , if we need to test before this gets back into 14.x. I could make the test job for small icu apply the patch before buildting/testing. That would allow us to enable testing for 14.x earlier.

@richardlau
Copy link
Member

Built locally to reproduce but all tests passed, likely because my system used Python3. I think that points in the direction of it being a Python2 problem as well.

PR with fix - nodejs/node#46263.

@richardlau , if we need to test before this gets back into 14.x. I could make the test job for small icu apply the patch before buildting/testing. That would allow us to enable testing for 14.x earlier.

@mhdawson Another option would be to always build with Python 3 (e.g. running with PYTHON=python3) for the small icu job?

@mhdawson
Copy link
Member

I've done that and enabled it for 14, job passed here https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1804_sharedlibs_smallicu_x64/35542/

I think we can probably now close this.

nodejs-github-bot pushed a commit to nodejs/node that referenced this issue Jan 21, 2023
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ruyadorno pushed a commit to nodejs/node that referenced this issue Feb 1, 2023
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 3, 2023
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit to nodejs/node that referenced this issue Mar 5, 2023
Refs: nodejs/build#2998

Small icu seems broken from 14.x since it uses
python2. Although main no longer supports python2
landing and backporting this change to the 14.x line would
allow us to simplify future backports as currently
the files are the same across lines.

Signed-off-by: Michael Dawson <mdawson@devrus.com>
PR-URL: #46263
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants