CI(openwrt): refactor build and test process

[1715173329]

This refactored the whole build & test process.

Cleaned up the Makefile, and switched to cmake build.

For CI, switched to OpenWrt's official build process, and build ipk for popular architectures. Also implemented runtime test for armv7 / aarch64 and i386 / amd64.

[codecov-commenter]

Codecov Report

Merging #1037 (3c2890e) into dev (ba8855f) will not change coverage. The diff coverage is n/a.

@@           Coverage Diff           @@
##              dev    #1037   +/-   ##
=======================================
  Coverage   20.74%   20.74%           
=======================================
  Files          47       47           
  Lines        8447     8447           
=======================================
  Hits         1752     1752           
  Misses       6695     6695           

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update ba8855f...3c2890e. Read the comment docs.

[hamishcoleman]

I think that there are some good things here, but I am concerned about accepting all of the change. It would be easier if it were broken down into smaller - functionally separate - changes that we could review one at a time.

For a start, you have switched the build to use the deprecated cmake system - the Makefiles are the preferred build process, so we dont want to increase the usage of the cmake.

[1715173329]

I think that there are some good things here, but I am concerned about accepting all of the change. It would be easier if it were broken down into smaller - functionally separate - changes that we could review one at a time.

Okay I will do it today.

For a start, you have switched the build to use the deprecated cmake system - the Makefiles #are the preferred build process, so we dont want to increase the usage of the cmake.

Cmake is the modern build system, I have no idea why deprecated it. Cmake is simpler than legacy autotools, more human readable, and more friendly to crossing-compile. It also compiles faster. But sure, I can switch to autotools back ;)

[1715173329]

Hi @hamishcoleman, is there any way to build test tools without actually executing them? It's impossible to execute them on the host as we are doing crossing-compile.

make[4]: Leaving directory '/home/buildbot/immortalwrt/build_dir/target-aarch64_cortex-a53_musl/n2n-ba8855fa71ca864e180876e0a10088a73b3ce1fa/tools'
scripts/test_harness.sh tests/tests_units.list
./tools/tests-auth >tests/tests-auth.out
/lib/ld-musl-aarch64.so.1: No such file or directory
Makefile:211: recipe for target 'test.units' failed
make[3]: *** [test.units] Error 255

[hamishcoleman]

Hi @hamishcoleman, is there any way to build test tools without actually executing them?

The default make target will build all the binaries - including the test tools

[hamishcoleman]

@1715173329 Can you point me to the documentation that you refer to for the openwrt official build process?

[1715173329]

@1715173329 Can you point me to the documentation that you refer to for the openwrt official build process?

https://github.com/openwrt/gh-action-sdk idk if it can be called as a "documentation ", but no more details available. It's pretty simple I think.

And this is how upstream uses it: https://github.com/openwrt/packages/tree/master/.github/workflows

[hamishcoleman]

Thank you for that pointer.

How are you going with splitting this PR into smaller ones so that each functional change can be separately addressed?

[1715173329]

The default make target will build all the binaries - including the test tools

Thanks. I found them located in tools, which is different from CMake that puts binaries into root.

How are you going with splitting this PR into smaller ones so that each functional change can be separately addressed?

All of these changes are around with OpenWrt, I have split them into different commits already. The latter commit needs the former one to work, I have no idea how to split this PR into even "smaller" ones.

[hamishcoleman]

The default make target will build all the binaries - including the test tools

Thanks. I found them located in tools, which is different from CMake that puts binaries into root.

Yes, the CMake files are not well maintained and have not always placed things in the right place.

How are you going with splitting this PR into smaller ones so that each functional change can be separately addressed?

All of these changes are around with OpenWrt, I have split them into different commits already. The latter commit needs the former one to work, I have no idea how to split this PR into even "smaller" ones.

While these are all related to OpenWRT, you are making several different changes all at once. Some of which we would want to discuss. If you cannot split this PR then we need to consider it all or nothing, which may not be what you were trying to achieve. In this case, let me add some review notes.

[1715173329]

Yes, the CMake files are not well maintained and have not always placed things in the right place.

Where CMake puts things it depends on CMakeLists, but I'm also not familiar with CMake system. These parts can be dropped.

[hamishcoleman]

Have you noticed that there are multiple build failures in the CI? Is there a known cause for that?

[1715173329]

Have you noticed that there are multiple build failures in the CI? Is there a known cause for that?

Yeah. It's caused by a feature AUTOREMOVE from OpenWrt, I'm looking for a way to fix it.

[hamishcoleman]

Each time you force push, it not only makes it annoying to see your change, but much more importantly, there is no comment or context for /why/ you make whatever change you made. If you simply add new commits, each one can have a command explaining the commit - before merging, we could easily squash the extra commits that this might cause (or, perhaps better, leave them so there is a track and trail for others who are attempting to understand why the code was written in the way it is)

[hamishcoleman]

I suspect that you are using some kind of automated yaml formatting tool - unless the tool can avoid making random whitespace changes between each commit, it makes it harder to see the changes that have happened. This is especially clear with the dancing between two different types of yaml string blocks depending on the length of the string - which makes the differences touch more lines than needed for the change.

[1715173329]

Uhhhh, different community has different rules. For me I just followed rules from OpenWrt (by default).

[1715173329]

I suspect that you are using some kind of automated yaml formatting tool - unless the tool can avoid making random whitespace changes between each commit, it makes it harder to see the changes that have happened. This is especially clear with the dancing between two different types of yaml string blocks depending on the length of the string - which makes the differences touch more lines than needed for the change.

Aha, are u sure? All changes were made by hand.

[hamishcoleman]

[...] This is especially clear with the dancing between two different types of yaml string blocks depending on the length of the string - which makes the differences touch more lines than needed for the change.

Aha, are u sure? All changes were made by hand.

No, it was a guess - based on other times I have seen this.

Ideally, try to minimise the number of lines your patch touches - so if a string block is already multi-line, just leaving it as a multi-line allows the patch to touch less lines.

(Obviously if a single-line string needs to be split for line-length reasons, this still should be done)

[1715173329]

Ideally, try to minimise the number of lines your patch touches - so if a string block is already multi-line, just leaving it as a multi-line allows the patch to touch less lines.

My $0.02 is to keep the final file as tiny as I can.

(Obviously if a single-line string needs to be split for line-length reasons, this still should be done)

Yeah, but this line isn't that so long and wrapping a new line doesn't really help (due to indentation.)

[1715173329]

Based on the comments above, the maintainers might not use openwrt for development, so I decide to close this PR, as maintaining something unfamiliar is not a good idea. feel free ping me if there's anyone really interested in this.