seL4 / ci-actions

CI GitHub actions for the seL4 repositories
https://sel4.systems
3 stars 13 forks source link

"Test with: <org>/<repo>#<id>" does no work across organizations when forking repos #184

Closed axel-h closed 2 years ago

axel-h commented 2 years ago

Using "Test with: <org>/<repo>#<id>" does no work across organizations, at least that is what I see in https://github.com/axel-h/seL4/pull/54 in the CI logs. There is "Test with: https://github.com/seL4/seL4_tools/pull/135" but looking at job run "Simulation (armv7a, clang)" run https://github.com/axel-h/seL4/runs/5053972234?check_suite_focus=true#logs is says

Manifest summary:
  -----------------
   sel4test-manifest: e4bbc0fd Updating default.xml                           (origin/master)
                seL4: c5b26abb Merge b3e7bfcaf15dc79af8d84ccfee10818d590780.. (pull/54/merge)
            musllibc: 3d6b939e aarch64_sel4: Use more supported asm directive (seL4/sel4)
           seL4_libs: d8ae2c44 Remove assert  in sys_io.c::sys_close()        (seL4/master)
  sel4_projects_libs: b687e3d5 trivial: Fixup compiler warnings               (seL4/master)
         sel4runtime: d935dd05 aarch32: Rename __aeabi_read_tp file (#15)     (seL4/master)
            sel4test: 62d8d068 scheduler: Increase timeout period when simu.. (seL4/master)
           util_libs: b0cedde6 trivial: resolve compiler warning in ethdriv.. (seL4/master)
              nanopb: 1466e6f9 Publishing nanopb-0.4.3                        (tags/0.4.3)
             opensbi: a98258d0 include: Bump-up version to 0.8                (tags/v0.8^0)
          seL4_tools: 1829e9c0 cmake: invoke python3 explicitly               (seL4/master)

But for seL4_tools it should have picked https://github.com/seL4/seL4_tools/pull/135

PRs on the seL4 organization work fine, for example in https://github.com/seL4/seL4/pull/759 the sam simulation job https://github.com/seL4/seL4/runs/5032065682?check_suite_focus=true says

Manifest summary:
  -----------------
   sel4test-manifest: e4bbc0fd Updating default.xml                           (origin/master)
                seL4: b02b04 RISC-V: reserve memory for SBI in device tree  
            musllibc: 3d6b939e aarch64_sel4: Use more supported asm directive (seL4/sel4)
           seL4_libs: d8ae2c44 Remove assert  in sys_io.c::sys_close()        (seL4/master)
  sel4_projects_libs: b687e3d5 trivial: Fixup compiler warnings               (seL4/master)
         sel4runtime: d935dd05 aarch32: Rename __aeabi_read_tp file (#15)     (seL4/master)
            sel4test: 62d8d068 scheduler: Increase timeout period when simu.. (seL4/master)
           util_libs: b0cedde6 trivial: resolve compiler warning in ethdriv.. (seL4/master)
              nanopb: 1466e6f9 Publishing nanopb-0.4.3                        (tags/0.4.3)
             opensbi: a98258d0 include: Bump-up version to 0.8                (tags/v0.8^0)
          seL4_tools: 9381ff3c elfloader, RISC-V: Move the elfloader during.. (pull/135/head)
lsf37 commented 2 years ago

I can't see the source of the description in your repo, but did you use the text seL4/seL4_tools#135 or the text https://github.com/seL4/seL4_tools/pull/135 in it? The action currently only understands the former even though GitHub displays both as the same. (I changed it in Kent's PR)

If it is that, I'll have a look whether I can change the parser to support both, then we don't have to think about it and it's often easier to just copy/paste a link.

axel-h commented 2 years ago

ah yes...that may explain it all. So maybe the regex could be extended to accept "https://github.com/<org>/<repo>/pull/<id>" as pattern also besides the current "<org>/<repo>#<id>". I'm currently playing with forked ci-action reps, so I'l try it out and make a PR then.

lsf37 commented 2 years ago

I don't remember exactly what I did, but I think it'll also need to process the format to convert it into the expected org/repo#id for the rest of the scripts to understand what's going on. Happy for you to give it a go, though.

axel-h commented 2 years ago

Seems I'm missing something here with a forked axel-h/ci-action to test my changes. There is a commit in the forked axel-h/sel4 to use my ci-action fork. And the sel4test simulation job https://github.com/axel-h/seL4/runs/5061359392?check_suite_focus=true tells me

Download action repository 'axel-h/ci-actions@pr-54' (SHA:001feb97172269f5107a6190f6760d6622)
...
Run axel-h/ci-actions/sel4test-sim@pr-54

But then what is running (inside the docker container) seems from 'sel4/ci-actions@master` and not from my branch. At least none of my printed message show up there. So what is the trick here to test this?

lsf37 commented 2 years ago

Docker containers are one more level of indirection, they don’t directly run what is in the repo. You first have to build and deploy them to dockerhub, then adjust the location/tag of the image to use your container in the corresponding action.yml in the ci-actions repo.

lsf37 commented 2 years ago

(There is an action in the ci-actions repo to do that deployment automatically on merge for the seL4 org, but that won’t work on a fork)

axel-h commented 2 years ago

Seems modifying sel4test-sim/action.yml and replacing image: 'docker://sel4/sel4test-sim:latest'by image: 'Dockerfile' might build the container on demand. Just the Dockerfile there no longer works.

lsf37 commented 2 years ago

For testing that is an option, but for production you don't usually want to do that, because some of these take long and the build is much more fragile than a fixed image.

The only action in this repo that currently uses Dockerfile directly is standalone-kernel (standalone compile of the verified platforms), and even for that one I'm thinking of making it uniform with the rest to avoid confusion.

lsf37 commented 2 years ago

Just the Dockerfile there no longer works.

The Dockerfile in sel4test-sim does work, but if you use it in the action it probably doesn't see the full context (the context of all Dockerfiles in the repo is the repo root, not the directory of the action -- see comment in the Dockerfile).

axel-h commented 2 years ago

Closing this, as this for for different repos. Extending the syntax is a different feature request actually