gap icon indicating copy to clipboard operation
gap copied to clipboard

add big-endian support to GH Actions

Open dimpase opened this issue 1 year ago • 3 comments

This is requested in https://github.com/gap-system/gap/blob/a2d1f49f54479a3896d4674d2ede6ad88abbb31d/.github/workflows/CI.yml#L151

A quick way may be found in https://github.com/wasm3/wasm3/blob/fa18e9ece4dd45371deac69ded32838470a55c1b/.github/workflows/tests.yml#L79

big-endian-i32-constant:
    runs-on: ubuntu-latest

    steps:
    - uses: actions/checkout@v3
    - name: Get the qemu container
      run: docker run --rm --privileged multiarch/qemu-user-static --reset -p yes
    - name: Run tests
      run: docker run --rm --interactive --mount type=bind,source=$(pwd),target=/host s390x/alpine sh -c "apk add build-base cmake git --update-cache && cd /host && mkdir build && cd build && cmake .. && cd .. && cmake --build build && echo wasm3 built && echo AGFzbQEAAAAGBgF/AEElCwcLAQdteWNvbnN0AwAACARuYW1lAgEA | base64 -d > myconst.wasm && echo ':get-global myconst' | ./build/wasm3 --repl myconst.wasm && echo ':get-global myconst' | ./build/wasm3 --repl myconst.wasm 2>&1 | grep 37"

(obviously the 1-line script in the last line will need adjustments, such as switching from apline to something less insane, and of course what's done there (i.e. building/testing, too.)

dimpase avatar Sep 25 '22 21:09 dimpase

@fingolfin

dimpase avatar Sep 25 '22 21:09 dimpase

Thank you for reminding me about this outdated TODO comment, I've submitted PR #5069 to remove it and others.

As explained on the support mailing list, I have no intention to invest any time on bringing such a CI test to live. Moreover, I also have zero interest in fixing any issues it might flag. Overall, I think it would be a net negative for this projective to try any of that. Even if someone else (e.g. you) provided a pull request, I still think it would be a net negative because now any CI run is slowed down by that test, and if it ever starts to fail, someone has to debug it.

So, all in all, I am against this. I'll leave this open for a time in case others want to comment, though.

fingolfin avatar Sep 25 '22 23:09 fingolfin

I thought that's what Bill wants - from Debian pov, they try to support many more platforms...

It's a reasonable downstream request, to test on a plarform they support.

One doesn't have to run all the CI tests on each PR. Optional tests like this may be factored out in a separate workflow, which can be triggered expicitly.

dimpase avatar Sep 25 '22 23:09 dimpase

I realise Debian might want to support many platforms, but we have limited person-power. Personally my preference is we don't try to support big-endian in any way unless someone was willing to promise to:

  • look at any CI failures within a week at most.
  • Investigate any bug reports from big endian users.
  • Actually fix any big-endian specific bugs.

Going even further (this is just my opinion, not GAP's!) -- GAP should explicitly say it does not support big-endian, and discourage users as strongly as possible. GAP is a big complicated maths system and people often publish results based on it. We have lots of types of tests (packages, much longer tests, memory checking tests), and I wouldn't want people using GAP on a CPU where these weren't all run and checked.

ChrisJefferson avatar Sep 26 '22 08:09 ChrisJefferson

I thought that's what Bill wants - from Debian pov, they try to support many more platforms...

It's a reasonable downstream request, to test on a plarform they support.

I disagree, and I can't think of any major project doing that.

But if a volunteer (e.g. Bill or you ?) stepped up to maintain that CI (basically following what @ChrisJefferson outlined), that would be an option.

One doesn't have to run all the CI tests on each PR. Optional tests like this may be factored out in a separate workflow, which can be triggered expicitly.

Not running all CI tests on all PRs just results in broken tests that nobody notices. I don't see this as useful; what's the point?

Actually, if someone wants this (weekly CI tests, as opposed to tests run on at every PR, and constantly monitored by the GAP Team), then they could easily set it up without even any need to involve the main GAP repository (just make a fork, set the CI up there, and regularly merge / rebase compared to the GAP master branch, via a cron job), couldn't they?

fingolfin avatar Sep 26 '22 08:09 fingolfin

I thought that's what Bill wants - from Debian pov, they try to support many more platforms... It's a reasonable downstream request, to test on a plarform they support.

I disagree, and I can't think of any major project doing that.

I don't think it's a common knowledge that it's easy to set up. Looking 10 years back, it took some time to get en mass adoption of Travis CI, so I guess it's not going to be instant now, either.

But if a volunteer (e.g. Bill or you ?) stepped up to maintain that CI (basically following what @ChrisJefferson outlined), that would be an option.

Can you convince Bill to use GitHub for this purpose, at least? :-)

One doesn't have to run all the CI tests on each PR. Optional tests like this may be factored out in a separate workflow, which can be triggered expicitly.

Not running all CI tests on all PRs just results in broken tests that nobody notices. I don't see this as useful; what's the point?

It's overtesting, to test absolutely everything on every platform. Perhaps 95% of bugs are platform-independent, anyway.

Actually, if someone wants this (weekly CI tests, as opposed to tests run on at every PR, and constantly monitored by the GAP Team), then they could easily set it up without even any need to involve the main GAP repository (just make a fork, set the CI up there, and regularly merge / rebase compared to the GAP master branch, via a cron job), couldn't they?

This ought to be done under GH gap-system organisation, still - in a separate repo, why not.

dimpase avatar Sep 26 '22 09:09 dimpase

why not

I think we already explained this multiple times now: because nobody volunteers to do it in a way that causes no extra work for the central project.

if you or someone else wants to step up for this, with a ready solution, we'll consider it. But anything that requires work on our side (such as convincing Bill of something) is out.

I am going to close this now, it is not productive to continue this discussion

fingolfin avatar Sep 26 '22 09:09 fingolfin

I volunteer, but I can't do it alone - my own reliability and motivation stopped passing CI lately :-).

Anyway, if Bill decides he can help here, this ought to be reopened.

dimpase avatar Sep 26 '22 09:09 dimpase

I volunteer, but I can't do it alone - my own reliability and motivation stopped passing CI lately :-).

Perfectly understandable :-).

Anyway, if Bill decides he can help here, this ought to be reopened.

I don't see why? Instead I'd expect a PR to be opened?

fingolfin avatar Sep 26 '22 10:09 fingolfin

I don't see why?

never mind, sorry. Indeed, a PR will do.

dimpase avatar Sep 26 '22 11:09 dimpase