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

Support Arm64 #77

Closed

Conversation

VladRassokhin
Copy link
Contributor

Fixes #67

@VladRassokhin
Copy link
Contributor Author

@basil @oleg-nenashev could you please take a look?

@basil
Copy link
Member

basil commented Oct 13, 2022

Per https://github.com/jenkins-infra/repository-permissions-updater/blob/5184ead375718f340d53732d12a0ed5b8164389b/permissions/component-winp.yml#L7 this component is not maintained by me individually but rather by the Jenkins core team. The fact that I have performed reviews and releases in the past should not be viewed as a commitment to perform reviews and releases in the future. I will not respond to ping requests if they are directed to me individually rather than to the Jenkins core team at large.

@basil
Copy link
Member

basil commented Oct 13, 2022

As a one-time courtesy I took a quick look at this. I followed the instructions at https://github.com/jenkinsci/winp/blob/e26179b0ae6a58d4e71ea2850b9304070ef9a97c/DEVELOPER.md to install MSbuild 15.0 and Microsoft Visual Studio 2017 with Windows XP support and BuildTools v140, then in the Developer Command Prompt for VS 2017 I was able to successfully run build.cmd cleanbuild on the main branch. With the changes from this PR, the build now fails with:

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.InvalidPlatform.Targets(22,7): error MSB8007: The Platform for project 'winp.vcxproj' is invalid. Platform='arm64'. You may be seeing this message because you are trying to build a project without a solution file, and have specified a non-default Platform that doesn't exist for this project. [C:\Users\basil\src\jenkinsci\winp\native\winp.vcxproj]

The error persisted even after I went back to Visual Studio 2017 setup and installed a bunch of ARM-related components. (Note: I have no idea how to do Windows development.) This would indicate that at the very minimum the documentation needs to be updated, if not the build itself.

If the documentation is updated to the point that a complete newcomer like me could successfully run the build and tests locally, and if a PR to jenkinsci/jenkins is created demonstrating that the incremental JAR file from this PR passes the existing Jenkins test suite, then I would consider merging and releasing this. I make no further commitment to working on this in a timely fashion, or at all. You are free to file a PR adding your GitHub username to https://github.com/jenkins-infra/repository-permissions-updater/blob/5184ead375718f340d53732d12a0ed5b8164389b/permissions/component-winp.yml#L7 if you want permissions to merge PRs and do releases to the Jenkins project's Artifactory server.

@VladRassokhin
Copy link
Contributor Author

The fact that I have performed reviews and releases in the past should not be viewed as a commitment to perform reviews and releases in the future. I will not respond to ping requests if they are directed to me individually rather than to the Jenkins core team at large.

That's comletely OK, thanks.

C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140\Microsoft.Cpp.InvalidPlatform.Targets(22,7): error MSB8007: The Platform for project 'winp.vcxproj' is invalid. Platform='arm64'. You may be seeing this message because you are trying to build a project without a solution file, and have specified a non-default Platform that doesn't exist for this project. [C:\Users\basil\src\jenkinsci\winp\native\winp.vcxproj]

Disclosure: I've actually not used build-native.cmd script, as I've set up separate msbuild build steps on TeamCity and used build.cmd copy_native && build.cmd maven only.

Probably those two lines should be modified to have msbuild 15+ in PATH:

set PATH=%PATH%;%ProgramFiles(x86)%\Microsoft Visual Studio\2017\BuildTools\MSBuild\15.0\bin\amd64
set VCTargetsPath=C:\Program Files (x86)\MSBuild\Microsoft.Cpp\v4.0\V140

I've no idea why they was in build.cmd in first place.

Full disclosure: I'm not deeply interested in any progres on this PR, as for IntelliJ Platform I've published artifact from my fork (with signed libraries) and successfuly using it.

@basil
Copy link
Member

basil commented Oct 15, 2022

Probably those two lines should be modified

This is your PR. If you think it should be modified, then modify it. If you are not going to respond to feedback, I will eventually close this PR.

@pbo-linaro
Copy link
Contributor

Hi everyone,
any news on this PR?
It would be great to see windows-arm64 support for jenkins by having native winp binaries/libraries.

@basil
Copy link
Member

basil commented Mar 28, 2024

#77 (comment)

@pbo-linaro
Copy link
Contributor

Thanks for your answer @basil.

I have been reading this comment, but there is still a question:
Would you accept to merge it even without having dedicated hardware? The winp README mentions the opposite, so that's why I'm asking.

@gounthar
Copy link

gounthar commented Mar 28, 2024

I know one of our valued contributors owns an aarch64 Windows machine, but that doesn't guarantee they will have the time to test it.

Would it be possible to arrange temporary access to an aarch64 Windows machine for testing purposes?

@pbo-linaro
Copy link
Contributor

It depends what you call by temporary. If that's once to test things, it might be possible. It you need it on a regular basis (i.e. new releases), this could be more challenging.

@basil
Copy link
Member

basil commented Mar 28, 2024

Would you accept to merge it

Regarding what I would accept to do, I already wrote in #77 (comment) that

As a one-time courtesy I took a quick look at this

That one-time courtesy has now been taken, so I would direct you to the second part of my message:

You are free to file a PR adding your GitHub username to https://github.com/jenkins-infra/repository-permissions-updater/blob/5184ead375718f340d53732d12a0ed5b8164389b/permissions/component-winp.yml#L7 if you want permissions to merge PRs and do releases to the Jenkins project's Artifactory server.

@pbo-linaro
Copy link
Contributor

Thanks for the information @basil.
I just have one concern for the implementation.
Even though VS2017 is supposed to support arm64, it's better to use VS2019 for this, as most of the issues have been solved. Would that be acceptable to update documentation to force this version as the minimal one accepted?

It seems like you were attached to the documentation mentioning VS2017, so I prefer to ask before engaging work...

@pbo-linaro
Copy link
Contributor

@VladRassokhin I plan to take a look at this again. Did you make further modifications on this? I understood you found a solution on your side, but if we can get a clean thing upstream, everybody will benefit from this :)

Would you agree that I start working from your version? (mentioning your name of course).

@basil
Copy link
Member

basil commented Mar 29, 2024

It seems like you were attached to the documentation mentioning VS2017, so I prefer to ask before engaging work...

I'm not attached to anything other than someone giving this component a modern stable and reproducible build/test system with clear documentation and regular CI builds.

@pbo-linaro
Copy link
Contributor

Appreciate your answer @basil, we're on the same page.

@VladRassokhin
Copy link
Contributor Author

VladRassokhin commented Apr 2, 2024

Did you make further modifications on this? I understood you found a solution on your side, but if we can get a clean thing upstream, everybody will benefit from this :)

Would you agree that I start working from your version? (mentioning your name of course).

@pbo-linaro No further modifications were made, as it works fine for me. You can do whatever you want with this code.

@pbo-linaro
Copy link
Contributor

Thanks for your answer @VladRassokhin. Got something nearly ready based on your work 👍.

@pbo-linaro
Copy link
Contributor

I posted a new PR (#112), based on this one. Feel free to close current one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support arm platform
4 participants