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

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

Closed
axel-h opened this issue Feb 3, 2022 · 10 comments

Comments

@axel-h
Copy link
Member

axel-h commented Feb 3, 2022

Using "Test with: <org>/<repo>#<id>" does no work across organizations, at least that is what I see in axel-h/seL4#54 in the CI logs. There is "Test with: seL4/seL4_tools#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 seL4/seL4_tools#135

PRs on the seL4 organization work fine, for example in seL4/seL4#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
Copy link
Member

lsf37 commented Feb 3, 2022

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
Copy link
Member Author

axel-h commented Feb 3, 2022

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
Copy link
Member

lsf37 commented Feb 3, 2022

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
Copy link
Member Author

axel-h commented Feb 4, 2022

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
Copy link
Member

lsf37 commented Feb 4, 2022

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
Copy link
Member

lsf37 commented Feb 4, 2022

(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
Copy link
Member Author

axel-h commented Feb 4, 2022

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
Copy link
Member

lsf37 commented Feb 4, 2022

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
Copy link
Member

lsf37 commented Feb 4, 2022

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
Copy link
Member Author

axel-h commented Feb 8, 2022

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

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

No branches or pull requests

2 participants