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

Add --without-intl builds to CI matrix #419

Closed
bnoordhuis opened this issue May 18, 2016 · 26 comments
Closed

Add --without-intl builds to CI matrix #419

bnoordhuis opened this issue May 18, 2016 · 26 comments

Comments

@bnoordhuis
Copy link
Member

Would it be possible to add ./configure --without-intl buildbots to the CI matrix? I volunteer to fix up the test suite if necessary.

See nodejs/node#6820 for background.

@jbergstroem
Copy link
Member

Flavour irrelevant? Linux?

@bnoordhuis
Copy link
Member Author

More is better but Linux + Windows seems like a good start.

@jbergstroem
Copy link
Member

Slightly off-topic but would you see shared builds in the same light?

@bnoordhuis
Copy link
Member Author

I'm not dead set against testing it but it's mainly intended for distro packagers, and they know how to help themselves.

Disabling intl (crypto too, for that matter) is more of an end user thing for people on low-end systems.

@jbergstroem
Copy link
Member

I'll set this up on our most redundant setup.

@bnoordhuis
Copy link
Member Author

--without-inspector would be good to have too, see nodejs/node#7258.

@bnoordhuis
Copy link
Member Author

Another one: nodejs/node#9041

@bnoordhuis
Copy link
Member Author

Would also be good for testing pull requests like nodejs/node#9040.

@TimothyGu
Copy link
Member

TimothyGu commented Oct 17, 2017

I'm tired of this issue being open and inactive for so long. For my purposes I've created https://github.com/TimothyGu/node-no-icu, which uses Travis CI and a local cronjob to run tests without ICU every 6 hours on master. Doubtless to say, the first build will certainly fail as soon as it finishes, though the second build with nodejs/node#16251 will probably be much more successful. PR CIs are run manually.

@gibfahn
Copy link
Member

gibfahn commented Oct 20, 2017

I'm tired of this issue being open and inactive for so long.

Would you like to set up the Jenkins job? This is just waiting on someone having the time, the only bit that needs special access is the initial job creation. I'm happy to clone the Linux test job and give you access.

@TimothyGu
Copy link
Member

@gibfahn I saw https://ci.nodejs.org/job/node-test-commit-linux-nointl/ has been created. I think this can now be closed.

@gibfahn
Copy link
Member

gibfahn commented Oct 21, 2017

I think this can now be closed.

Is it actually run in the main flows? We should keep this open till that job is good to go. I also don't think running on that many platforms is a good idea, we have a lot of variations to test, so one or two platforms each is probably best.

@TimothyGu
Copy link
Member

@gibfahn In that case I'd be happy to be given the permissions to modify the job.

@gibfahn
Copy link
Member

gibfahn commented Oct 22, 2017

@TimothyGu I've given nodejs/Collaborators configure access. I guess we can always revoke it if this becomes part of the main flows.

What I think we need is a job that runs a variety of configurations, but only on one platform each (if it turns out we need more coverage then we can expand).

Things I've seen asked for (cc/ @danbev, who I'm sure has some requests):

@TimothyGu
Copy link
Member

@gibfahn I've done the following modifications to node-test-commit-linux-nointl:

  • Rename it to node-test-commit-nointl
  • Only run it on ubuntu1610-x64, which seems to be one of the most stable machine labels.

Things still needed to be done:

  • Make node-test-commit-nointl part of node-test-commit
  • Add the other configurations

Something I'm worried about however, is if we eventually stuff all nointl builds into one machine label (i.e. ubuntu1610-x64), that label will be overused while other stable machine labels are left vacant. This will become even more of a problem once builds using other configuration flags (nossl, etc.) are added. Is there a way to provide a list of acceptable nodes for Jenkins to choose from randomly (like a load balancer)?

@gibfahn
Copy link
Member

gibfahn commented Oct 22, 2017

Is there a way to provide a list of acceptable nodes for Jenkins to choose from randomly (like a load balancer)?

Yes and no, but that's a bigger question, so opened a separate issue: #934

@rvagg
Copy link
Member

rvagg commented Oct 23, 2017

Added some notes in nodejs/node#16130 (comment) about how we might go about testing a shared openssl without requiring dedicated CI machines for it, this would also work for the other shared libs we support (http-parser, libuv, zlib, cares — basically what Linux distro packagers are dynamically linking against.)

@gibfahn
Copy link
Member

gibfahn commented Oct 26, 2017

How about this as a proposal:

Add a pipeline to this repo that takes a set of sets of config flags as a build parameter. For each set it runs a make run-ci on Linux, macOS, and Windows. We can then have a single job that can run an arbitrary set of build types (as long as they just differ in config flags).

Example:

CONFIG_FLAGS='"--without-intl" "--enable-static" "--without-ssl"'

would run three different builds, one for --without-intl, one for --enable-static, and one for --without-ssl, each on the three main platforms.

Will attempt to raise PR if I get time.

@rvagg
Copy link
Member

rvagg commented Nov 4, 2017

perhaps we could make a subset of nodejs/Collaborators for this kind of thing, nodejs/Collaborators is very broad and it might be best if we restrict it to just people that are interested enough in doing this kind of work; like a nodejs/jenkins-job-config or something that doesn't give full access? maybe we can talk about this at our next meeting.

@rvagg
Copy link
Member

rvagg commented Nov 4, 2017

moving my comments to #972

@maclover7
Copy link
Contributor

@TimothyGu Can you confirm that https://ci.nodejs.org/job/node-test-commit-nointl/ works ok? If so, probably makes sense to close this issue, and then maybe move to a different one to discuss --without- flags in general

@TimothyGu
Copy link
Member

@maclover7 Last time I checked, the job works fine. However, I really want it to get into the daily build routine (doesn't have to be in node-test-commit, but executed frequent enough for us to notice issues about it).

@rvagg
Copy link
Member

rvagg commented Mar 6, 2018

Oh, hey, I think I missed this issue the first time around. We have infrastructure in place to set variations like this in https://ci.nodejs.org/job/node-test-commit-linux-containered/, each of the builds in there just have variations of CONFIG_FLAGS being passed in, this would fit really well. If we go ahead and ditch older Alpine then we have more than enough processing power on our Docker hosts to make these go just as quick as the others.

Is it as simple as CONFIG_FLAGS=--without-intl? Is there anything else special that I'm not seeing in node-test-commit-nointl? I can add a builder for that pretty easily if that's all there is to it and it'll get run on every commit.

@rvagg
Copy link
Member

rvagg commented Mar 6, 2018

I think I missed this issue the first time around

heh .. just noticed that I've been very active in this thread already last year .. I have no memory of this 😬

@TimothyGu
Copy link
Member

@rvagg

Is it as simple as CONFIG_FLAGS=--without-intl?

Yes.

@rvagg
Copy link
Member

rvagg commented Mar 7, 2018

Done, this lives in node-test-commit-linux-containered along with the other ./configure permutation builds. It's run inside Ubuntu 16.04 containers: https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_withoutintl_x64/

I've restricted it to >= Node 6.

I've also added the following sanity check after everything runs to make sure the binary is as we expect it to be (we have something like this for all the variations):

INTL_OBJECT="$(out/Release/node -pe 'typeof Intl')"
echo "Intl object type: $INTL_OBJECT"
if [ X"$INTL_OBJECT" != X"undefined" ]; then
  FAIL_MSG="Has an Intl object, exiting"
  echo $FAIL_MSG
  echo "1..1" > ${WORKSPACE}/test.tap
  echo "not ok 1 $FAIL_MSG" >> ${WORKSPACE}/test.tap
  exit -1
fi
PROCESS_VERSIONS_INTL="$(out/Release/node -pe process.versions.icu)"
echo "process.versions.icu: $PROCESS_VERSIONS_INTL"
if [ X"$PROCESS_VERSIONS_INTL" != X"undefined" ]; then
  FAIL_MSG="process.versions.icu not undefined, exiting"
  echo $FAIL_MSG
  echo "1..1" > ${WORKSPACE}/test.tap
  echo "not ok 1 $FAIL_MSG" >> ${WORKSPACE}/test.tap
  exit -1
fi

We don't quite have enough containers to do more than 2 node-test-commit-linux-containered's simultaneously now because this adds an extra build for each one. However I've just pulled out a heap of Alpine containers from active use so we have capacity to spin up more containers on our existing Docker hosts. I'll get to that in the next few days.

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

6 participants