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

Add support for loongarch64 #2691

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

glaubitz
Copy link
Collaborator

@glaubitz glaubitz commented Dec 2, 2024

This Fixes #2684

@schaefi
Copy link
Collaborator

schaefi commented Dec 3, 2024

Thanks for digging into this topic. For the code itself can you please also adapt the tests accordingly ? Thanks much.
For merging a new architecture support at least one integration test build is needed. I assume you do this primarily for Debian ? Our debian/ubuntu integration tests can be found here:

The descriptions you can find in the code here

For adding loongarch64 we need a test at this point. I have to admit I never played with that architecture nor do I have access to any board that implements it, nor do I know which distribution builds for loongarch64

If this is all still in flux and providing a test build is not so easy we at least need a clean unit checklist for the changed code paths.

Thanks much

Copy link
Collaborator

@schaefi schaefi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see my former comment

@schaefi
Copy link
Collaborator

schaefi commented Dec 3, 2024

@glaubitz also looked at the information from your hackweek project. So it seems like you are already able to build something for loongarch64 based on tumbleweed ?

@glaubitz
Copy link
Collaborator Author

glaubitz commented Dec 3, 2024

@glaubitz also looked at the information from your hackweek project. So it seems like you are already able to build something for loongarch64 based on tumbleweed ?

Yes, openSUSE for loongarch64 is currently being bootstrapped here.

The reason I want to add loongarch64 support to KIWI is to be able to build openSUSE installation images for loongarch64.

@glaubitz
Copy link
Collaborator Author

glaubitz commented Dec 3, 2024

I just had a look at the tests and I think implementing these for loongarch64 will be too much.

Is there a reason why we need those for loongarch64 but not riscv64?

@Conan-Kudo
Copy link
Member

We need them for RISCV64 too, the fact that they are missing is not good either.

@schaefi
Copy link
Collaborator

schaefi commented Dec 3, 2024

I just had a look at the tests and I think implementing these for loongarch64 will be too much.

I'm talking about covering your code changes with proper unit tests and providing at least one integration test build which you probably do anyway because I assume you have built something with your current set of bootstrap packages. I'm afraid we cannot merge code that is not covered by some sort of testing. If the current state of the distro does not allow you to run a build test in the buildservice (which is the service used for the integration test builds) then we can let that go for the moment, but covering code changes by a unit test is a must have in kiwi to keep full freedom to do changes in all parts of the code without regressions. I believe it is not too much of a burden to add that with regards to the code changes done in this PR.

Thanks

@schaefi
Copy link
Collaborator

schaefi commented Dec 3, 2024

We need them for RISCV64 too, the fact that they are missing is not good either.

nobody ever used kiwi to build anything for riscv64. This is basically an unsupported arch for kiwi as we have not a single build for any riscv64 board. I'm not surprised that this is missing.

@schaefi
Copy link
Collaborator

schaefi commented Dec 3, 2024

We need them for RISCV64 too, the fact that they are missing is not good either.

nobody ever used kiwi to build anything for riscv64. This is basically an unsupported arch for kiwi as we have not a single build for any riscv64 board. I'm not surprised that this is missing.

And to be more precise here; There was one guy in the past who added a patch in a similar fashion than Adrian does it now for loongarch64. The changes to kiwi regarding riscv were approved by me without further asking and we now have code for this architecture without any tests and without any integration test. This is basically dead code and nobody knows if it still works. I would say it doesn't.

This is the reason why I'm picky on adding new architecture support to come with an image build that actually proves it also works. There is also a tendency that people who added some-arch support vanishes and leave others with arch specific code alone. You cannot read a test you cannot test the board because it was only done half-way. I myself know about x86, aarch64 and s390. I have no idea about ppc, riscv and loongarch64 and that means other people need to stay responsible for the work they do. Those who are willing to write tests and come up with integration tests are usually also there to fix stuff when things starts to break... and they always break.

Just in case you are asking yourself why I'm so picky on tests when it comes to new architectures, this is the reason and I hope for your understanding

@schaefi
Copy link
Collaborator

schaefi commented Dec 9, 2024

@glaubitz I fixed the unit test in your fork and pushed a commit. I hope this is ok ?

@schaefi
Copy link
Collaborator

schaefi commented Dec 10, 2024

I had a short call with Adrian, he will try to add a test build to our build-tests/ area and once this exists I'm going to add a new project at Virtualization:Appliances:Images:Testing_loongarch64 such that we have some test build to refer to. Once this is done we are ready for a merge :) Thanks

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.

Please add support for LoongArch
3 participants