Support for windows-arm64 #112
Conversation
.github/workflows/build-test.yml
Outdated
There was a problem hiding this comment.
Could you use ci.jenkins.io instead of GitHub Actions?
There was a problem hiding this comment.
Technically, I don't see any problem to it. However, advantage of GitHub Actions is that it manages its own infrastructure, so there is nothing to install and maintain. In more, if someone forks this, they benefit from the same CI on their fork, without having to open a PR first.
I'm not sure if I can create a new job description by myself, or if someone from infra team should help on this?
There was a problem hiding this comment.
Most Jenkins projects are built by Jenkins on ci.jenkins.io, doing the same for this project would be better IMO.
There is already a job for this repo, not sure what description you'd like to add/change, let me know I will be able to help you with that (I'm part of Jenkins Infrastructure team).
There was a problem hiding this comment.
From what I see on https://ci.jenkins.io/job/jenkinsci-libraries/job/winp, only java part of the code is analyzed/built (an not the native, which comes as prebuilt binaries). The concerned jenkinsFile is here: https://github.com/jenkinsci/winp/blob/master/Jenkinsfile.
@basil, I noticed you edited this file. Is there any reason why native build was not made on jenkins infra previously? It seems to me that is was kept out of ci.jenkins.io for some reason.
There was a problem hiding this comment.
@pbo-linaro Please stop pinging me about this repository. I know nothing more about its history than you do. I am unsubscribing from this thread.
There was a problem hiding this comment.
(I'm not a maintainer though, my approval won't help getting this PR merged)
There was a problem hiding this comment.
I think it would be a nice first step to get the new binaries for aarch64 with a GitHub Action, and then open another PR to convert it to Jenkins.
My $0.02.
There was a problem hiding this comment.
Thanks to both of you.
My only concern with jenkins ci is not the pipeline file, but the debug of the agent involved, plus probably some things to install (else, I assume it would have already been done). From my experience, without direct access to the machine (which I won't probably have as a random contributor), it's just painful. Advantage of GitHub Actions is that runner is fully documented and stateless.
From there, we need to find someone who could officially review and merge this.
The suggestion from Basil (in previous PR) was to add myself as jenkins core developer. However, I'm not very fond of this, an excellent recent example of this being the xz exploit that affected the entire open source ecosystem. Beyond that, if I was a maintainer, I would remove the prebuilt binaries from this repo, and ensure they are built and pushed through a automated pipeline instead.
There was a problem hiding this comment.
Advantage of GitHub Actions is that runner is fully documented and stateless.
As are the stateless Kubernetes container agents and stateless VM agents provided by the Jenkins infrastructure team. As noted above, this is the Jenkins project, so we are asking you to use Jenkins rather than GitHub Actions for CI and to work with the Jenkins infrastructure team to do so. I trust that the reason behind this request is obvious.
(Note that the Jenkins project does use GitHub Actions for CD, as opposed to CI, though gated on a passing Jenkins CI build, and the same would apply here. In fact, there is all the more reason to use CD because of the complicated compiler prerequisites for building this component.)
So to be clear, the desired end state is to work with the Jenkins infrastructure team to provide a CI build running on Jenkins and a CD running on GitHub Actions, with no prebuilt binaries, as is standard throughout the project.
The suggestion from Basil (in previous PR) was to add myself as jenkins core developer.
No, my suggestion was to take ownership of the winp component alongside the Jenkins core team, not to join the Jenkins core team. Among other things, you won't be able to run CI builds on ci.jenkins.io without commit access to the repository in question, and you'll need to run those builds while testing the CI job for this component.
However, I'm not very fond of this, an excellent recent example of this being the xz exploit that affected the entire open source ecosystem.
If you're concerned about this, we could just give you commit access to this repository for the purpose of testing CI builds in PRs, and not give you release access to the winp component. Once CD is implemented, we can remove your commit access and enable exclusive CD in repository-permissions-updater so that only the remaining committers (i.e., the Jenkins core team) can perform CD releases. In any case, we wouldn't adopt a release in Jenkins core and ship it to our 200,000 Jenkins users if it contained a binary that we didn't compile ourselves, so there is no need to worry about that.
I would remove the prebuilt binaries from this repo, and ensure they are built and pushed through a automated pipeline instead.
Yes, please do this. As I've stated several times in the previous thread, there isn't an "existing maintainer" to do this work, so you would be signing up to do this work yourself. And there isn't an "existing maintainer" to ask questions about the existing code either, so you would have to reverse engineer past decisions yourself as well. But we (including members of the Jenkins infrastructure team) are here to help you navigate Jenkins project-specific community details, such as our CI/CD process and permission system.
There was a problem hiding this comment.
I think we agree on the direction this should take.
No problem to do some preliminary work (CI + remove prebuilt binaries), before adding arm64 support, I just need to check if it goes within my time allocation on this project.
There was a problem hiding this comment.
From my perspective this PR is trying to do too much in a single PR. There are a few separate problems here, one being lack of proper CI/CD that doesn't use any prebuilt binaries, and the other being missing ARM support. Unfortunately, the lack of proper CI/CD support precludes the implementation of the second, so I think we should first address that in a dedicated PR (or PRs) and then add ARM support once the existing technical debt is addressed. See my post here for more details.
I agree with this, can we split the CI bit to another PR, if you need to include the Visual Studio upgrade in there then it would be ok but if you don't I wouldn't. Personally I'm fine with GitHub actions for this, this may be the Jenkins project but this project isn't really Jenkins specific. |
Hi Mark, I'm not a VS user neither (only its toolchain MSVC). A review is always welcome :) Thanks |
|
Fixed issues in VS files reported by @MarkEWaite. |
|
I'd love this to get through, I now have a Windows on Arm laptop that I would like to do Jenkins development on. Is there anything I can help with here? |
|
@slide The next steps, which can be done by any interested contributor, are to create a new PR that enables CI and JEP-229 CD for this repository, extracting whatever portions of this PR are minimally necessary for that task. If it is desired to file one PR for CI and a separate one for CD, that can also be done. It is preferred that the PR for CI use Jenkins rather than GitHub Actions; if this requires custom software to be installed on our agent images, it is expected we would file a ticket at https://github.com/jenkins-infra/helpdesk/issues to get a consensus from the infrastructure team regarding whether or not that is feasible, falling back to GitHub Actions only if the infrastructure team votes against prioritizing the work to update our agent images. If the PR involves changes to the |
|
@slide Forgot to mention in my last post that any precompiled binaries should be deleted in that PR, and the build should be driven from Maven, i.e., the entrypoint should be |
|
I'm now trying to reenable windows pipeline, and fix what is missing to get it to run. For now, my user does not seem to be allowed to modify Jenkins file (see details on new PR: #115). If someone can help me to be able to run a pipeline by myself, you're welcome! |
|
I've triggered it |
|
In short: I identified that VS can't be installed on nanoserver windows images. |
|
Just posted expected PR to work on build/ci part of this: #116. Comments are welcome 👍! |
|
Let's talk about the test plan for this. Can you confirm that the latest revision builds on ARM64 with Also please run an end-to-end test with Jenkins? That can be done by opening up a PR in https://github.com/jenkinsci/jenkins referencing the incremental build from this PR, for example: diff --git a/bom/pom.xml b/bom/pom.xml
index 6984a542f7..de6a630710 100644
--- a/bom/pom.xml
+++ b/bom/pom.xml
@@ -268,7 +268,8 @@ THE SOFTWARE.
<dependency>
<groupId>org.jvnet.winp</groupId>
<artifactId>winp</artifactId>
- <version>1.30</version>
+ <!-- TODO https://github.com/jenkinsci/winp/pull/112 -->
+ <version>1.31-rc370.68f7fb_273c8d</version>
</dependency>
<dependency>
<groupId>org.kohsuke</groupId> |
Some error code were found on windows-arm64 machine, and we now ignore a code for specific protected processes. The unit test concerned lists all processes on the machine, which makes it hard to reproduce and reliable.
|
This last push rebased work to enable windows-arm64. |
|
Thank you so much for your work @pbo-linaro. 🙏 |
There was a problem hiding this comment.
Tests are working on x64/arm64 platforms, in debug and release mode.
Great! Can you please run an end-to-end test with Jenkins? That can be done by opening up a PR in https://github.com/jenkinsci/jenkins referencing the incremental build from this PR, for example:
diff --git a/bom/pom.xml b/bom/pom.xml
index 6984a542f7..de6a630710 100644
--- a/bom/pom.xml
+++ b/bom/pom.xml
@@ -268,7 +268,8 @@ THE SOFTWARE.
<dependency>
<groupId>org.jvnet.winp</groupId>
<artifactId>winp</artifactId>
- <version>1.30</version>
+ <!-- TODO https://github.com/jenkinsci/winp/pull/112 -->
+ <version>1.31-rc361.b_66b_d9b_9ea_3b_</version>
</dependency>
<dependency>
<groupId>org.kohsuke</groupId>From my perspective, I'll be ready to approve this change once the incremental has been tested in context in Jenkins and once #117 is completed.
|
Created PR for jenkins integration here: jenkinsci/jenkins#9229 |
There was a problem hiding this comment.
Created PR for jenkins integration here: jenkinsci/jenkins#9229
Great! From my perspective, I'll be ready to approve and merge this PR once changes to native code become releasable through the completion of #117.
|
I don't see the engineering rationale to block merging this to force completion of #117. In any case, if someone makes a new release (manually or automatically), it would be good to see those changes integrated. Or maybe your goal is to force anyone to complete #117 before merging anything. In this case, you can simply archive this repository, and mark it as read only. |
There was a problem hiding this comment.
I don't see the engineering rationale to block merging this to force completion of #117.
Allow me to explain the rationale, then. The rationale is as follows: we don't allow regressions. Prior to #116, the repository was in a state such that changes to native code were not releasable but changes to non-native code (such as #69) were releasable. #116 (in the absence of #117) regressed the ability to release changes to non-native code, but this temporary situation seemed acceptable under the assumption that #117 would be completed shortly thereafter. If #117 is not completed in a timely fashion, then I will revert #116 to fix this regression, and it can be reintegrated once the fix for #117 has been prepared.
In any case, if someone makes a new release (manually or automatically), it would be good to see those changes integrated.
Agreed! Unfortunately, I am not aware of anyone who has stepped up to set up a (manual or automated) release environment for this repository.
Or maybe your goal is to force anyone to complete #117 before merging anything.
The completion of #117 (or the appearance of a maintainer with the environment and time commitment to do manual releases) isn't a prerequisite before merging anything, but it is a prerequisite before merging changes to the native code, since otherwise such changes aren't releasable and prevent the release of changes to non-native code (a regression).
In this case, you can simply archive this repository, and mark it as read only.
I can, but I see no reason why I should. On the contrary, archiving the repository might prevent someone from filing a pull request that fixes #117 and unblocks the release of native changes.
This PR adds windows-arm64 support to winp component, plus some additional enhancements:
Tests were run on windows-x64 and arm64.
Based on #77, from @VladRassokhin.
Fixes #67
Closes #77
Testing done
Submitter checklist