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

ansible: add ICU to sharedlibs containers #2677

Merged
merged 1 commit into from
Jun 22, 2021
Merged

Conversation

richardlau
Copy link
Member

Add versions of ICU to the sharedlibs containers. Intended to be used
to build and test Node.js against a system ICU:
e.g. for ICU 65

$ export PKG_CONFIG_PATH=$ICU65DIR/lib/pkgconfig
$ export LD_LIBRARY_PATH=$(pkg-config --variable=libdir icu-i18n)
$ export CONFIG_FLAGS='--with-intl=system-icu --verbose'
$ make run-ci

Add versions of ICU to the sharedlibs containers. Intended to be used
to build and test Node.js against a system ICU:
e.g. for ICU 65
```
$ export PKG_CONFIG_PATH=$ICU65DIR/lib/pkgconfig
$ export LD_LIBRARY_PATH=$(pkg-config --variable=libdir icu-i18n)
$ export CONFIG_FLAGS='--with-intl=system-icu --verbose'
$ make run-ci
```
@richardlau
Copy link
Member Author

This is deployed onto the containers on

  • test-digitalocean-ubuntu1804_docker-x64-1
  • test-digitalocean-ubuntu1804_docker-x64-2
  • test-softlayer-ubuntu1804_docker-x64-1

I've not made any CI job changes yet.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

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

there's some opportunities for some clever bash in here, but maybe keeping it simple-ish is good enough

are we going to add regular jobs for each of these? we're going to need to have a hard look at capacity I think

@richardlau
Copy link
Member Author

there's some opportunities for some clever bash in here, but maybe keeping it simple-ish is good enough

Admittedly I didn't try but the docs for RUN (https://docs.docker.com/engine/reference/builder/#run) says Docker runs /bin/sh -c which I believe is dash (and not bash) on Ubuntu systems. We could use SHELL instead to specifically execute bash, but I think we'll end up with something less readable as that needs to be in JSON form.

are we going to add regular jobs for each of these? we're going to need to have a hard look at capacity I think

My initial thought was to add one job (well subjob of the linux-containered job) that would parse out the minimum ICU version from tools/icu/icu_versions.json and setup the "system" version of ICU to build based on that. The various versions of ICU in this PR is because each release line has a different minimum ICU -- I added the current minimum ICU for the release lines (12.x, 14.x, 16.x and master -- for 14.x we're planning on restoring the minimum ICU back to 65 but it's currently 67, nodejs/node#39068) plus ICU 69 (V8 9.2, nodejs/node#38990).

The other way we could do it is to test as many versions of ICU as possible (i.e. a new subjob for each ICU version and then VersionSelector.groovy to filter them by release line) but that will be more maintenance for the Build team (keeping VersionSelector.groovy updated, managing several icu* labels in Jenkins) and would be several new (sub)jobs which I would then share your concerns over capacity.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@richardlau richardlau merged commit 3aaa723 into nodejs:master Jun 22, 2021
@richardlau richardlau deleted the icu branch June 22, 2021 11:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants