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

added VersionSelectorScript.groovy to select builders for Node version #1217

Merged
merged 25 commits into from
May 30, 2018

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 6, 2018

Following on from #1153

node-version-jenkins-plugin is now installed on ci.nodejs.org and is being used in node-test-commit-linux-arm, node-test-commit-linux-containered and node-test-commit-linux to select the right builders to run. So we have some ARM and containered specific options in here. The script also has a "buildType" so that it can do slightly different things for release as for test (and other), e.g. centos6 shouldn't be used for Node 4 in release but it should be used for everything for test.

mhdawson

This comment was marked as off-topic.

@mhdawson
Copy link
Member

mhdawson commented Apr 9, 2018

Also is this just for release or is it going to be used for the test ci as well? Was going to look at turning off testing for BE >=8 but this may take care of it if it will apply to the test ci.

@rvagg rvagg force-pushed the rvagg/VersionSelectorScript.groovy branch from 8543e21 to 3bfe64a Compare April 10, 2018 08:11
@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2018

@mhdawson re ppc, cause the script is already set up for this I've just edited node-test-commit-plinux and (1) enabled the plugin (under git advanced) and (2) inserted the Groovy Matrix Strategy script to download this script and execute it for filtering and now ppcbe is not running for Node >=8 like on ci-release. If you want it to run later than that then we can modify it to be different for buildType == 'release' vs test.

https://ci.nodejs.org/job/node-test-commit-plinux/jobConfigHistory/showDiffFiles?timestamp1=2018-04-04_06-56-16&timestamp2=2018-04-10_04-51-40

Regarding docs, in theory the only thing that should need to be done is edit this script. Unfortunately I'm finding that the Groovy script is caching downloads so it doesn't fetch updates straight away and I'm having to edit the URL inside the jobs to add a unique ?x on the end to fetch the new version. Not ideal, so I'm going to have to get my Groovy on and figure out a non-caching fetch. Then, if I get that working, it's only editing this script that's needed to change the rules. Of course there are many edge-cases, like the centos gcc one I just implemented that involves an extra layer of hackery, but at least we can remove all version checking into one place.

@rvagg rvagg force-pushed the rvagg/VersionSelectorScript.groovy branch 2 times, most recently from f9cff6f to e739e77 Compare April 10, 2018 09:51
@rvagg
Copy link
Member Author

rvagg commented Apr 10, 2018

def code = new URL(scriptUrl).getText(useCaches: false, requestProperties: ['Connection': 'close'])

these options seem to be enough to force proper single-use fetching 👍

@mhdawson
Copy link
Member

@rvagg, thanks for the update. Having the test use the same rules as for release for PPCBE works for now. Thanks :)

gibfahn

This comment was marked as off-topic.

@rvagg rvagg force-pushed the rvagg/VersionSelectorScript.groovy branch 5 times, most recently from cdf46d5 to 88142ce Compare April 12, 2018 05:44
@rvagg
Copy link
Member Author

rvagg commented Apr 12, 2018

@gibfahn I've experimented with two new variations of this that make it a bit more grokkable

  1. everything is a regex and names have been simplified: (https://github.com/nodejs/build/blob/88142ce670883a31eaecddb2f7b4bf74ddd9b88e/jenkins/scripts/VersionSelectorScript.groovy)
  2. turn it into a DSL, the "build exclusions" are in the form of an array of arrays with 3 elements: regex, build type, version match (current HEAD)

2 is what's being used now, I quite like it cause it reads easily, we end up being able to define rules like:

  [ /^centos5/,                       anyType,     gte(8)  ],
  [ /^centos6/,                       releaseType, lt(8)   ],
  [ /centos[67]-(arm)?(64|32)-gcc48/, anyType,     gte(10) ],
  [ /centos[67]-(arm)?(64|32)-gcc6/,  anyType,     lt(10)  ],
  [ /centos6-32-gcc6/,                releaseType, gte(10) ], // 32-bit linux for <10 only
  [ /^ubuntu1804/,                    anyType,     lt(10)  ], // probably temporary
  [ /^ubuntu1204/,                    anyType,     gte(10) ],

I've addressed your other comments with these changes I think, two outstanding ones:

  • re the -1 default, if something messes up then all builders will run, there's a >=4 in there that makes that happen. Basically if you submit a repo to it that it can't figure out the node version from src/node_version.h then it'll just throw its hands up and guess that you probably didn't intend rules to apply to this one. I could make it fail with a dramatic throw or something if you think this is a big enough deal.
  • The .toString().toInteger() thing is necessary because it's a ParameterValue or somesuch from Jenkins, so the toString() extracts the plain value out of it.

gibfahn

This comment was marked as off-topic.

@gibfahn
Copy link
Member

gibfahn commented Apr 12, 2018

2 is what's being used now, I quite like it cause it reads easily, we end up being able to define rules like:

Yeah that looks really good, 💯 on that.

re the -1 default, if something messes up then all builders will run, there's a >=4 in there that makes that happen.

Ahh okay, makes sense. I don't think throwing is really better than running on all platforms, having it default to all platforms so you can see what works, and use it to inform your decision of how to change the list makes a lot of sense to me. Maybe add a comment to that line though.

gibfahn

This comment was marked as off-topic.

gibfahn

This comment was marked as off-topic.

gibfahn

This comment was marked as off-topic.

@rvagg rvagg force-pushed the rvagg/VersionSelectorScript.groovy branch from 337a1ed to fe83359 Compare April 23, 2018 12:43
@rvagg rvagg force-pushed the rvagg/VersionSelectorScript.groovy branch from fe83359 to 3984214 Compare April 23, 2018 12:54
Add exclusion for Node.js built as shared library 

Don't run on versions less than 9
@maclover7
Copy link
Contributor

@rvagg This should be safe to land, right? It's already been running on public CI for a couple of weeks now

@rvagg
Copy link
Member Author

rvagg commented May 30, 2018

yeah, but we're referencing this branch now in a bunch of places so I need to go find all those references and point them to master!

@rvagg rvagg merged commit e680feb into master May 30, 2018
@refack
Copy link
Contributor

refack commented May 31, 2018

moved node-test-commit-linux to master

@gibfahn gibfahn deleted the rvagg/VersionSelectorScript.groovy branch May 31, 2018 19:58
@refack refack restored the rvagg/VersionSelectorScript.groovy branch May 31, 2018 20:01
@refack
Copy link
Contributor

refack commented May 31, 2018

Please don't delete branch until we're sure Jenkins is branch-use free.

@refack
Copy link
Contributor

refack commented May 31, 2018

Just run this in the Script Console and fixed all the jobs

import org.jenkinsci.plugins.GroovyScriptMES
import hudson.matrix.MatrixProject

for(item in Jenkins.instance.getAllItems(MatrixProject.class)) {
  e = item.getExecutionStrategy()
  if (e in GroovyScriptMES) {
    println e.secureScript.getScript().split("\n", 2)[0]
  	println item.absoluteUrl + 'configure'
    println '-----------------'
  }
}

@refack refack deleted the rvagg/VersionSelectorScript.groovy branch May 31, 2018 21:38
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.

5 participants