search

eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 720 forks source link

Github->Jenkins PR Trigger Comments need to support jdk8 #662

Closed AdamBrousseau closed 6 years ago

AdamBrousseau commented 6 years ago

related #660

We currently support the following PR builds via Github PR comments.

You can trigger these builds via PR commit comment Jenkins compile Jenkins compile zLinux Jenkins compile pLinux Jenkins test sanity Jenkins test sanity zlinux Jenkins test sanity plinux Jenkins test extended Jenkins test extended zlinux Jenkins test extended plinux

Here is the regex from the zLinux Sanity build: .*jenkins test sanity(.*zlinux.*)?(?! xlinux)(?! plinux).* Each job has a slightly different regex which will allow that job to run if the comment matches the regex

We need to update the regex's to support more JDK versions (i.e. 8,10 for the immediate future). The current regex 'model' is harder to maintain because each job has to maintain the list of platforms it is not interested in. The alternate would be to have a regex that looks for all or <my_platform>. This same problem will apply to JDK versions once we add 8,10.

Final regex example

.*\bjenkins\s+test\s+sanity\b\s*($|\n|depends\s+.*|(all|([a-z]+,)*zlinux(,[a-z]+)*)\s*($|\n|depends\s+.*|all|(jdk[0-9]+,)*jdk8(,jdk[0-9]+)*)(\s+depends.*)?)

AdamBrousseau commented 6 years ago

Propose we update the regex to something like this: .*\bjenkins\b\s+\btest\b\s+\bsanity\b\s+(\ball\b|\bzlinux\b)\s+(\ball\b|\bjava9\b).* A new comment for zLinux Java9 sanity would be one of the following jenkins test sanity all all jenkins test sanity all java9 jenkins test sanity zlinux all

DanHeidinga commented 6 years ago

Is it possible to support jenkins test sanity as a short form for jenkins test sanity all all?

AdamBrousseau commented 6 years ago

.*\bjenkins\b\s+\btest\b\s+\bsanity\b(\n|$|\s+(\ball\b|\bzlinux\b)\s+(\ball\b|\bjava9\b).*) Should allow jenkins test sanity as well as jenkins test sanity all all

The other problem with this regex is that you can only request 1/all platforms (or versions). I.e. you can't get 2/3 platforms.

We could allow comma separated list of platforms or versions with this .*\bjenkins\b\s+\btest\b\s+\bsanity\b\s*(\n|$|(all|([a-z]+,)*zlinux(,[a-z]+)*)\s+(all|(java[0-9]+,)*java9(,java[0-9]+)*))

This would allow any of the following (and more)

pshipton commented 6 years ago

If jenkins test sanity zlinux could be used as short form for jenkins test sanity zlinux all then the "new syntax" would be compatible with the old.

mstoodle commented 6 years ago

should we be using the word "java" or "jdk" in these commands?

my vote is for jdk...

mstoodle commented 6 years ago

might be easier for casual reading if there were a p= (platforms) and j= (jdk) prefixes for those arguments to make it easier to understand "all all" / etc. but it's just a thought. unless others agree, I won't push for it.

I guess it look would look verbose in simple cases, though.

AdamBrousseau commented 6 years ago

I think this addresses Pete's and both Mark's suggestions .*\bjenkins\b\s+\btest\b\s+\bsanity\b\s*(\n|$|(p=all|([a-z]+,)*zlinux(,[a-z]+)*)\s*(\n|$|j=all|(jdk[0-9]+,)*jdk9(,jdk[0-9]+)*))

DanHeidinga commented 6 years ago

@AdamBrousseau For the lazy, can you supply what the new commands would look like?

AdamBrousseau commented 6 years ago

Example

keithc-ca commented 6 years ago

Are the expressions case-sensitive?

BTW, occurrences of \b\s+\b can be just \s+.

mstoodle commented 6 years ago

another option would "allpfm" and "alljdk" in case that is more attractive

AdamBrousseau commented 6 years ago

Matches case insensitively and supports regular expressions (from GitHub Pull Request Builder)

.*\bjenkins\s+test\s+sanity\b\s*(\n|$|(p=all|([a-z]+,)*zlinux(,[a-z]+)*)\s*(\n|$|j=all|(jdk[0-9]+,)*jdk9(,jdk[0-9]+)*))

DanHeidinga commented 6 years ago

Since we're bikeshedding anyway, the = is kind of ugly. Is there a single world that would fill in for "all"? ie: use jdk as a short hand for jdk=all?

keithc-ca commented 6 years ago

With commas separating the platforms and jdk versions, I don't see a need for the p= or jdk= prefixes.

keithc-ca commented 6 years ago

Anybody in favour of supporting jenkins help?

mstoodle commented 6 years ago

+1 to the "ugly" comment. As I mentioned before, it wasn't a strong suggestion and seems to add little value (if only one is specified, the entries in the other make it pretty clear which one is "all"). if both are specified, it isn't really important to know which is which. It's just that "all all" looks weird to me.

pshipton commented 6 years ago

All in favour of going back to the following? Note all of these do the same thing (when zlinux,plinux are the only platforms and jdk8,jdk9 are the only supported jdks).

AdamBrousseau commented 6 years ago

Updated #851 with README changes for trigger comment