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.

Sign up for GitHub

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

enable additional platforms in CI #1121

Open
wants to merge 1 commit into
base: riscv
Choose a base branch
from
Open

enable additional platforms in CI #1121

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 27 additions & 3 deletions .github/workflows/spike-openocd-tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Build Spike and run a couple of debug tests.

name: Test OpenOCD against 2 spike configurations
name: Test OpenOCD against 4 spike configurations

env:
SPIKE_REPO: https://github.com/riscv-software-src/riscv-isa-sim.git
Expand Down Expand Up @@ -115,7 +115,8 @@ jobs:
--gcc /opt/riscv/toolchain/bin/riscv-none-elf-gcc \
--gdb /opt/riscv/toolchain/bin/riscv-none-elf-gdb \
--sim_cmd /opt/riscv/spike/bin/spike \
--server_cmd $GITHUB_WORKSPACE/src/openocd
--remotelogfile-enable \
--server_cmd "$GITHUB_WORKSPACE/src/openocd -d3"
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would prefer not to add -d3 here. The reason is: d3 could change OpenOCD behavior (there are many cases of if (debug_level < 3) return; ).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

  1. I'm strongly against not testing -d3 . This is the mode when we use when run this testsuite internally. And this is the mode we use when we ask users when there are problems.
  2. The fact that there is these kinds of if statement in the codebase is a major problem, but I think it is a separate one. If it comes to that - such if statements should be eradicated. At least from riscv codebase for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the mode when we use when run this testsuite internally.

Exactly. After this change there will be no testing of OpenOCD without -d3.

Anyway, I don't have a strong opinion on this so I'll approve the change regardless.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@JanMatCodasip , @MarekVCodasip do you have an opinion on the matter? The concern raised by @en-sc is valid.

I'm inclined to extend testing even more by enabling both variants: "-d3" is enabled and "-d3" is omitted. Tests may be run a little bit slower now, however I think that we are better safe than sorry

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm inclined to extend testing even more by enabling both variants

Please, don't. This is pre-commit testing. I believe it's better to run such tests at least in post-commit.

Copy link
Collaborator

@JanMatCodasip JanMatCodasip Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There will always remain a little risk that adding -d3 will change behavior -- IMO very small but still present.

My recommendation would be to:

    1. Remove suspicious/risky statements if (debug_level < XYZ ) return; (in a separate merge request).
    1. Pick one verbosity level for the tests that run in the CI on per-commit basis (I prefer -d3 slightly more but don't feel strongly).
    1. Optionally, if you like: Run the tests with the other verbosity level, too, but less frequently. For example:
    1. Optionally, alternate approach: Use different logging level on different targets. This would at least introduce more variability into the testing.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@en-sc

Please, don't. This is pre-commit testing. I believe it's better to run such tests at least in post-commit.

I don't believe we should reduce amount when creating a commit in open source. We already had few regressions that could be detected by a more rigorous testing. This commit is a direct response on one of such regression. I don't mind waiting 40 minutes instead of 20 to get testing results. I don't think this is a bit problem.

@JanMatCodasip

Remove suspicious/risky statements if (debug_level < XYZ ) return; (in a separate merge request)

The thing is it is not that simple and may require a significant effort. Things are getting more complicated since the decision to use such approach was promoted in upstream several months ago. So unless the policy regarding such condition is changed in upstream - the issue shall persist. But we definitely should revise existing codebase regarding the matter.

Pick one verbosity level for the tests that run in the CI on per-commit basis (I prefer -d3 slightly more but don't feel strongly).

My pick would be -d3 too since usually this logging level allows to debug test.

Optionally, if you like: Run the tests with the other verbosity level, too, but less frequently.

I'll investigate this . Thanks for the idea.

Optionally, alternate approach: Use different logging level on different targets. This would at least introduce more variability into the testing.

As for messing up with different logging levels, I believe this may be an over complication. However, your suggestion regarding splitting test into multiple job may help with this one.


- name: Run Spike64-2 Tests
if: success() || steps.spike32-tests.conclusion == 'failure'
Expand All @@ -125,7 +126,30 @@ jobs:
--gcc /opt/riscv/toolchain/bin/riscv-none-elf-gcc \
--gdb /opt/riscv/toolchain/bin/riscv-none-elf-gdb \
--sim_cmd /opt/riscv/spike/bin/spike \
--server_cmd $GITHUB_WORKSPACE/src/openocd
--remotelogfile-enable \
--server_cmd "$GITHUB_WORKSPACE/src/openocd -d3"

- name: Run Spike64-2-hwthread Tests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now that the number of of tests has increased, it may be worth to split them to multiple jobs that would then run in parallel, leading to overall shorter test time.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, this is definitely a way to go. Will investigate how to do this.

Copy link
Collaborator

@JanMatCodasip JanMatCodasip Oct 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It looks to me that the current job can be split to multiple parallel jobs quite easily using strategy-matrix feature of Github Actions.

Here is a concrete example: https://github.com/HonzaMat/PyOpenocdClient/blob/main/.github/workflows/integration_tests.yml#L7

if: success() || steps.spike32-tests.conclusion == 'failure'
run: |
cd riscv-tests/debug
./gdbserver.py targets/RISC-V/spike64-2-hwthread.py --print-failures \
--gcc /opt/riscv/toolchain/bin/riscv-none-elf-gcc \
--gdb /opt/riscv/toolchain/bin/riscv-none-elf-gdb \
--sim_cmd /opt/riscv/spike/bin/spike \
--remotelogfile-enable \
--server_cmd "$GITHUB_WORKSPACE/src/openocd -d3"

- name: Run Spike32-2-hwthread Tests
if: success() || steps.spike32-tests.conclusion == 'failure'
run: |
cd riscv-tests/debug
./gdbserver.py targets/RISC-V/spike32-2-hwthread.py --print-failures \
--gcc /opt/riscv/toolchain/bin/riscv-none-elf-gcc \
--gdb /opt/riscv/toolchain/bin/riscv-none-elf-gdb \
--sim_cmd /opt/riscv/spike/bin/spike \
--remotelogfile-enable \
--server_cmd "$GITHUB_WORKSPACE/src/openocd -d3"
JanMatCodasip marked this conversation as resolved.
Show resolved Hide resolved

- name: Archive test logs
# Proceed even if there was a failed test
Expand Down