Support PullRequest builds from other repos #1354

AdamBrousseau · 2018-03-06T15:22:42Z

Determine if build is a PR
Automatically checkout PR and any dependent PRs
Support dependent changes from other repos

[ci skip]

Issue #660

Signed-off-by: Adam Brousseau adam.brousseau88@gmail.com

TODO

Update README
Open Eclipse Bugzilla to request Jenkins Webhook be added to openj9-omr repo
- Resolved

DanHeidinga · 2018-03-06T15:41:41Z

buildenv/jenkins/common/build

+            openjdk_bool = true
+            break
+        default:
+            error "Unable to determine sourece repo for PullRequest"


sourece -> source

AdamBrousseau · 2018-03-06T20:15:33Z

adam test sanity plinux jdk9

AdamBrousseau · 2018-03-06T20:25:27Z

adam test sanity plinux jdk9 depends eclipse/omr#2357 ibmruntimes/openj9-openjdk-jdk8#58

AdamBrousseau · 2018-03-06T20:48:44Z

Regular OpenJ9 PR
OpenJ9 PR with 2 dependent changes
OpenJDK8 PR with dependent 2 changes

Do we want to protect against people specifying dependent changes from the incorrect version of an openjdk-extensions repo?
eg. jenkins test sanity all jdk9 depends ibmruntimes/openj9-openjdk-jdk8#123

pshipton · 2018-03-06T21:10:06Z

Do we want to protect against people specifying dependent changes from the incorrect version of an openjdk-extensions repo?

I think so. Better than ignoring the dependent change and running the build anyway. How would it work? Perhaps running a job that immediately fails with an explanation of the problem.

AdamBrousseau · 2018-03-08T16:10:55Z

adam test sanity plinux jdk9 depends eclipse/omr#2357 ibmruntimes/openj9-openjdk-jdk8#58

AdamBrousseau · 2018-03-08T16:13:23Z

adam test sanity plinux jdk9 depends eclipse/omr#2357 ibmruntimes/openj9-openjdk-jdk9#108

AdamBrousseau · 2018-03-08T16:15:33Z

Dependent JDK change with correct SDK version
Dependent JDK change with wrong SDK version

AdamBrousseau · 2018-03-08T17:16:27Z

@vsebe for review

vsebe · 2018-03-08T18:54:45Z

desk review

AdamBrousseau · 2018-03-08T19:03:03Z

updated

AdamBrousseau · 2018-03-08T19:04:17Z

adam test sanity plinux jdk9 depends eclipse/omr#2357

AdamBrousseau · 2018-03-08T19:07:20Z

recent adjustment to switch-case still works
https://ci.eclipse.org/openj9/view/Sandbox/job/tmp_PullRequest-Sanity-JDK9-linux_ppc-64_cmprssptrs_le-OpenJ9/26/console

vsebe · 2018-03-08T19:55:26Z

buildenv/jenkins/common/build

+
+        // Setup PR_IDs for dependent changes
+        for (DEPEND in DEPENDS_ARRAY) {
+            String REPO  = DEPEND.substring(DEPEND.indexOf("/") + 1, DEPEND.indexOf("#"));


extra space before "="

vsebe · 2018-03-08T19:57:07Z

buildenv/jenkins/common/build

+        echo "No dependent changes found"
+    } else {
+        // Gather everything after KEYWORD. Assumes a whitespace after KEYWORD, hence +1
+        def DEPENDS_BLOB  = COMMENT.substring(COMMENT.indexOf(KEYWORD) + KEYWORD.length() + 1 , COMMENT.length())


extra space before "="

vsebe · 2018-03-08T19:59:36Z

buildenv/jenkins/common/build

+        checkout_pullrequest(OPENJDK_PR)
+    }
+
+    sh 'bash ./get_source.sh'


Inconsistency: use of single quotes instead of double quotes.

- Determine if build is a PR - Automatically checkout PR and any dependent PRs - Simplify compile_only PRs to be the same process as Build jobs - Support dependent changes from other repos - Verify OpenJDK dependent changes are the correct repo version [ci skip] Issue eclipse-openj9#660 Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

- Remove build badges for PR builds - Simplify PR build section - Indicate PR builds are replicated per REPO, per SPEC and per sdk VERISON [ci skip] Issue eclipse-openj9#660 Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

AdamBrousseau · 2018-03-08T21:58:11Z

updated

vsebe · 2018-03-08T22:06:29Z

LGTM

AdamBrousseau · 2018-03-09T16:43:53Z

Ready for committer review @pshipton or @DanHeidinga

keithc-ca · 2018-03-12T16:30:24Z

I'm not convinced this is a good direction to go. Rather than adjust the infrastructure to manage pull requests that must be coordinated across multiple repositories, I think we should aim to have pull requests that can be merged sequentially (first omr then openj9, or vice-versa). It may require a little more thought and work on the part of the author of the PRs, but I am confident that those PRs can always be constructed so there is an order they can be merged without build breakage at any intermediate state.

It can also get very complicated: consider a PR that is connected to openj9-openjdk changes. Currently this PR will only let one specify dependencies on one extension repo. If changes are required for jdk8, it's likely it also affects jdk9 (and jdk10, etc. - something not supported by the current proposal): where do we draw the line? I say that line should be at zero dependencies.

AdamBrousseau · 2018-03-12T17:50:18Z

@keithc-ca I agree that not having/needing dependent changes is a good idea. However, this PR is not about adding support for dependent changes, it is simply extending PR builds to other repos, which in turn extends the dependent change support. Also, there is an open issue (#827) to extend the "depends" support to also work with branches rather than just open PRs.

AdamBrousseau · 2018-03-14T21:41:28Z

If we want to discuss the validity of having dependent changes can we open a separate issue where the team leads can discuss? I don't think that conversation should be a blocker to this PR.
@keithc-ca @DanHeidinga

keithc-ca · 2018-03-14T21:55:10Z

On the contrary, I don't think we should be complicating things (like merging this PR would) until we have agreed that we want to consider PRs that come with dependencies.

AdamBrousseau · 2018-03-15T21:43:37Z

Keep in mind, we already support having pull requests with dependent changes. The purpose of this PR is extending PR Build functionality to the extensions repos. The only added functionality is the protection against cross-version dependent changes, which you could argue should be in a different PR as we actually have this vulnerability today, without these changes.

keithc-ca · 2018-03-15T22:14:54Z

I still don't think we should be doing this. If (and that a big if) we want to consider dependent PRs involving the extension repos, then it needs to allow specification of all the extension repos, not at most one.

AdamBrousseau · 2018-03-15T22:16:34Z

https://github.com/eclipse/openj9/blob/master/buildenv/jenkins/README.md#dependent-changes

pshipton · 2018-03-21T03:06:43Z

There are times when having dependent changes is just easier. Perhaps they could be committed in multiple steps to avoid the dependencies, but why would we want to do that dance when the changes can be committed (very closely) together. Seems like a waste of developer, committer, and machine time to avoid a very small timing hole. Dependent omr changes can be problematic because of the omr acceptance build, but there is no such issue for committing dependencies between openj9 and the extensions repos.

With this PR, builds for all the extensions repos can be done, it just requires multiple commands. This could be improved in the future, if there is nothing better to do, but its not a requirement that needs to block merging this.

DanHeidinga · 2018-03-21T13:26:06Z

@pshipton I agree with you this is the right approach for now but have to take issue with:

but why would we want to do that dance when the changes can be committed (very closely) together. Seems like a waste of developer, committer, and machine time to avoid a very small timing hole.

That small timing hole leaves the repos in a broken state. It stops tools like git bisect from running. It leaves other developers with potentially broken repos.

Dependent changes externalize the cost of delivering a change from the dev making the change to everyone consuming the project. It's not a great trade off.

Unfortunately, there are some dependent changes that we can't avoid. We need this as a tool for those (rare) cases.

Committers should continue to encourage all developers to stage their changes so we don't need this kind of tool.

pshipton · 2018-03-21T13:44:24Z

@DanHeidinga agreed. Good point, the timing hole I mentioned can sometimes be avoided by committing the changes in the correct order, even if they are within a few seconds of each other.

DanHeidinga

This looks reasonable to me. The doc issues can be addressed in a separate PR.

DanHeidinga · 2018-03-21T13:26:55Z

buildenv/jenkins/README.md

@@ -72,11 +74,9 @@ You can also request a "Compile & Sanity" build from the extensions repos or ope
 - Ex. Dependent change in OpenJDK Pull Request `#789`
    - `Jenkins test sanity depends ibmruntimes/openj9-openjdk-jdk9#789`


Isn't this line missing all jdk9 as well?

DanHeidinga · 2018-03-21T13:39:31Z

buildenv/jenkins/README.md

@@ -72,11 +74,9 @@ You can also request a "Compile & Sanity" build from the extensions repos or ope
 - Ex. Dependent change in OpenJDK Pull Request `#789`
    - `Jenkins test sanity depends ibmruntimes/openj9-openjdk-jdk9#789`
 - Ex. Dependent changes in OMR and OpenJDK
-    - `Jenkins test sanity depends eclipse/omr#123 ibmruntimes/openj9-openjdk-jdk9#789`
+    - `Jenkins test sanity all jdk9 depends eclipse/omr#123 ibmruntimes/openj9-openjdk-jdk9#789`


This fixes one of these - aren't other commands incorrect as well?

keithc-ca · 2018-03-21T13:47:17Z

I hadn't thought of the use cases where it simply makes life easier for a committer - those make sense. Ultimately, a committer has the responsibility to ensure that merges go smoothly and if this helps, I'm fine with it.

AdamBrousseau force-pushed the support_extensions_prs branch 3 times, most recently from 931ea0a to dc882da Compare March 6, 2018 15:29

DanHeidinga added the comp:build label Mar 6, 2018

DanHeidinga reviewed Mar 6, 2018

View reviewed changes

AdamBrousseau force-pushed the support_extensions_prs branch 6 times, most recently from 4bcf1c2 to 788216a Compare March 6, 2018 20:15

AdamBrousseau force-pushed the support_extensions_prs branch from 788216a to 5d48e55 Compare March 6, 2018 20:40

AdamBrousseau mentioned this pull request Mar 6, 2018

[ci skip] WIP DO NOT MERGE update readme ibmruntimes/openj9-openjdk-jdk8#58

Closed

AdamBrousseau mentioned this pull request Mar 6, 2018

Add JDK8 PR builds to repos OpenJ9, OpenJ9-OMR and OpenJ9-OpenJDK-JDK8 and nightly builds #660

Closed

32 tasks

AdamBrousseau force-pushed the support_extensions_prs branch from 2ecb544 to 520641d Compare March 8, 2018 16:06

AdamBrousseau force-pushed the support_extensions_prs branch 8 times, most recently from 0c55da9 to e86edc8 Compare March 8, 2018 17:08

AdamBrousseau force-pushed the support_extensions_prs branch from bd541f9 to d8bf6e0 Compare March 8, 2018 17:15

AdamBrousseau changed the title ~~WIP: Support PullRequest builds from other repos~~ Support PullRequest builds from other repos Mar 8, 2018

AdamBrousseau force-pushed the support_extensions_prs branch from d8bf6e0 to 63fd034 Compare March 8, 2018 19:01

vsebe suggested changes Mar 8, 2018

View reviewed changes

AdamBrousseau added 2 commits March 8, 2018 16:57

Add README info about PR builds from other repos

5646fa3

- Remove build badges for PR builds - Simplify PR build section - Indicate PR builds are replicated per REPO, per SPEC and per sdk VERISON [ci skip] Issue eclipse-openj9#660 Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>

AdamBrousseau force-pushed the support_extensions_prs branch from 63fd034 to 5646fa3 Compare March 8, 2018 21:57

vsebe approved these changes Mar 8, 2018

View reviewed changes

DanHeidinga reviewed Mar 21, 2018

View reviewed changes

pshipton approved these changes Mar 21, 2018

View reviewed changes

pshipton merged commit 81b5342 into eclipse-openj9:master Mar 21, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support PullRequest builds from other repos #1354

Support PullRequest builds from other repos #1354

AdamBrousseau commented Mar 6, 2018 •

edited

Loading

DanHeidinga Mar 6, 2018

AdamBrousseau commented Mar 6, 2018

AdamBrousseau commented Mar 6, 2018

AdamBrousseau commented Mar 6, 2018

pshipton commented Mar 6, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

vsebe commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

vsebe Mar 8, 2018

vsebe Mar 8, 2018

vsebe Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

vsebe commented Mar 8, 2018

AdamBrousseau commented Mar 9, 2018

keithc-ca commented Mar 12, 2018

AdamBrousseau commented Mar 12, 2018

AdamBrousseau commented Mar 14, 2018

keithc-ca commented Mar 14, 2018

AdamBrousseau commented Mar 15, 2018

keithc-ca commented Mar 15, 2018

AdamBrousseau commented Mar 15, 2018

pshipton commented Mar 21, 2018

DanHeidinga commented Mar 21, 2018

pshipton commented Mar 21, 2018

DanHeidinga left a comment

DanHeidinga Mar 21, 2018

DanHeidinga Mar 21, 2018

keithc-ca commented Mar 21, 2018

		@@ -72,11 +74,9 @@ You can also request a "Compile & Sanity" build from the extensions repos or ope
		- Ex. Dependent change in OpenJDK Pull Request `#789`
		- `Jenkins test sanity depends ibmruntimes/openj9-openjdk-jdk9#789`

Support PullRequest builds from other repos #1354

Support PullRequest builds from other repos #1354

Conversation

AdamBrousseau commented Mar 6, 2018 • edited Loading

TODO

DanHeidinga Mar 6, 2018

Choose a reason for hiding this comment

AdamBrousseau commented Mar 6, 2018

AdamBrousseau commented Mar 6, 2018

AdamBrousseau commented Mar 6, 2018

pshipton commented Mar 6, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

vsebe commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

AdamBrousseau commented Mar 8, 2018

vsebe Mar 8, 2018

Choose a reason for hiding this comment

vsebe Mar 8, 2018

Choose a reason for hiding this comment

vsebe Mar 8, 2018

Choose a reason for hiding this comment

AdamBrousseau commented Mar 8, 2018

vsebe commented Mar 8, 2018

AdamBrousseau commented Mar 9, 2018

keithc-ca commented Mar 12, 2018

AdamBrousseau commented Mar 12, 2018

AdamBrousseau commented Mar 14, 2018

keithc-ca commented Mar 14, 2018

AdamBrousseau commented Mar 15, 2018

keithc-ca commented Mar 15, 2018

AdamBrousseau commented Mar 15, 2018

pshipton commented Mar 21, 2018

DanHeidinga commented Mar 21, 2018

pshipton commented Mar 21, 2018

DanHeidinga left a comment

Choose a reason for hiding this comment

DanHeidinga Mar 21, 2018

Choose a reason for hiding this comment

DanHeidinga Mar 21, 2018

Choose a reason for hiding this comment

keithc-ca commented Mar 21, 2018

AdamBrousseau commented Mar 6, 2018 •

edited

Loading