Skip to content

Commit

Permalink
os: use 'assert'-proc in run scripts
Browse files Browse the repository at this point in the history
  • Loading branch information
rite committed Feb 11, 2025
1 parent 817403a commit e19c503
Show file tree
Hide file tree
Showing 31 changed files with 126 additions and 165 deletions.
2 changes: 1 addition & 1 deletion repos/os/run/ahci_block.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec x86
assert {[have_spec x86]}

create_boot_directory
build {
Expand Down
19 changes: 7 additions & 12 deletions repos/os/run/assert_nic.inc
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
#
# there are no nic driver packages for these targets
#
if {[have_board rpi3] ||
[have_board imx53_qsb_tz]} {

puts "Run script is not supported on this platform."
exit 0
}
assert {![have_board rpi3]}
assert {![have_board imx53_qsb_tz]}

#
# these targets would require extra setup on the autopilot which is not desired
#
if {[get_cmd_switch --autopilot] && ([have_spec linux] ||
[have_board zynq_qemu] ||
[have_board virt_qemu_riscv])} {

puts "Autopilot mode is not supported on this platform."
exit 0
if {[have_cmd_arg --autopilot]} {

This comment has been minimized.

Copy link
@rite

rite Feb 11, 2025

Author Owner

It is pretty common case that assertions depend on whether the run script is executed in "autopilot mode". Of course, this could be combined into the actual expression of the assertion, but IMHO the readability suffers. Another option is to add a assert_if-proc. This way, the log would contain all the information of why the test has been aborted without requiring to specify a additional message and without sacrificing readability. Any opinions?

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

I agree to your statement about readability. Your version looks fine to me.

assert {![have_board linux] &&
![have_board zynq_qemu] &&
![have_board virt_qemu_riscv]} \
Autopilot mode is not supported on this platform.

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

The ability to supplement an optional message to an assertion is good. But instead of passing the message as varargs, let us better use a single (optional) argument, like so:

proc assert { expression { msg "" } }  {
  ...

This comment has been minimized.

Copy link
@rite

rite Feb 13, 2025

Author Owner

Actually this is how I did it before I came across TCLs ::control::assert, but then I changed it to match its interface. But tbh, I don't know why ::control::assert uses varargs. So a single, optional argument it is ;) I added two fixup commits to my feature branch.

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 14, 2025

Thanks for sharing the rationale, and of course for the adjustment.

}
6 changes: 1 addition & 5 deletions repos/os/run/assert_qemu_nic.inc
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,4 @@ source ${genode_dir}/repos/os/run/assert_nic.inc
#
# the test should be run isolated as it would disturb normal network operation
#
if {![have_include power_on/qemu]} {

puts "Run script is not supported on this platform."
exit 0
}
assert {[have_include power_on/qemu]}
2 changes: 1 addition & 1 deletion repos/os/run/block_tester.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec linux
assert {[have_spec linux]}

#
# Check used commands
Expand Down
8 changes: 3 additions & 5 deletions repos/os/run/bomb.run
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,9 @@ if {[have_include "power_on/qemu"]} {
}
}

if {[have_spec pistachio]} {
# The bomb test triggers a kernel assertion at kernel/src/api/v4/tcb.h,
# line 727, which remains unfixed since the kernel is no longer developed.
puts "Run script does not support Pistachio"
exit 0
assert {![have_spec pistachio]} {
The bomb test triggers a kernel assertion at kernel/src/api/v4/tcb.h,
line 727, which remains unfixed since the kernel is no longer developed.
}

# prevent hitting the socket-descriptor limit on Linux
Expand Down
11 changes: 3 additions & 8 deletions repos/os/run/cpu_balancer.run
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,9 @@ build {
server/cpu_balancer app/cpu_burner app/top server/dynamic_rom
}

if {![have_include "power_on/qemu"]} {
puts "Run script is not supported on this platform"
exit 0
}
# foc in principle supports migration, but due to bug #4357 foc is not tested
if {![have_spec nova] && ![have_spec sel4]} {
puts "Run script is not supported on this platform"
exit 0
assert {[have_include power_on/qemu]}
assert {![have_spec foc]} {

This comment has been minimized.

Copy link
@rite

rite Feb 11, 2025

Author Owner

I opted for the described requirement, rather then what is currently implemented (which for example doesn't consider base-hw). Not sure if this is correct though...

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

That's good. Thanks for pointing out this inconsistency.

foc in principle supports migration, but due to bug #4357 foc is not tested
}

set cpu_width 4
Expand Down
13 changes: 4 additions & 9 deletions repos/os/run/cpu_bench.run
Original file line number Diff line number Diff line change
@@ -1,14 +1,9 @@
if {[get_cmd_switch --autopilot]} {
if {[have_include "power_on/qemu"]} {
puts "\nRun script does not support Qemu.\n"
exit 0
}
if {[have_cmd_arg --autopilot]} {
assert {![have_include power_on/qemu]} \
Autopilot mode is not supported on this platform.
}

if {[have_board linux]} {
puts "\n Run script is not supported on this platform. \n";
exit 0
}
assert {![have_board linux]}

build { core init timer lib/ld test/cpu_bench }

Expand Down
20 changes: 12 additions & 8 deletions repos/os/run/demo.run
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
if {[have_board imx6q_sabrelite] ||
[have_board imx7d_sabre] ||
[have_board zynq_usrp_e31x] ||
([get_cmd_switch --autopilot] && [have_board linux]) ||
([get_cmd_switch --autopilot] && [have_include "power_on/qemu"])} {
puts "Run script does not support this platform."
exit 0
assert {[have_recipe pkg/[drivers_interactive_pkg]]} "
Recipe for 'pkg/[drivers_interactive_pkg]' not available.
"

assert {![have_board imx6q_sabrelite]}

This comment has been minimized.

Copy link
@rite

rite Feb 11, 2025

Author Owner

Ideally, this is covered by the [have_recipe]-assertion above. Is this correct? If so, I'd like to get rid of these lines.

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

That's an excellent idea.

This comment has been minimized.

Copy link
@rite

rite Feb 13, 2025

Author Owner

I'll provide a fixup commit.

assert {![have_board imx7d_sabre]}
assert {![have_board zynq_usrp_e31x]}

if {[have_cmd_arg --autopilot]} {
assert {![have_board linux] && ![have_include power_on/qemu]} \
Autopilot mode is not supported on this platform.
}

create_boot_directory
Expand Down Expand Up @@ -217,7 +221,7 @@ close $launchpad_config_fd

build_boot_image [list {*}[build_artifacts] launchpad.config]

if {[get_cmd_switch --autopilot]} {
if {[have_cmd_arg --autopilot]} {
run_genode_until {\[init -> scout\] png is.*\n} 40

grep_output {(requests resources: )|(Error)}
Expand Down
28 changes: 14 additions & 14 deletions repos/os/run/fb_bench.run
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
if {[have_board imx7d_sabre] ||
[have_board imx6q_sabrelite] ||
[have_board rpi3] ||
[have_board zynq_qemu] ||
[have_board zynq_usrp_e31x] ||
[have_board imx53_qsb_tz] ||
[have_board imx53_qsb] && [have_spec foc]} {
puts "\n Run script is not supported on this platform. \n";
exit 0
}
assert {[have_recipe pkg/[drivers_interactive_pkg]]} "
Recipe for 'pkg/[drivers_interactive_pkg]' not available.
"

assert {![have_board imx7d_sabre]}

This comment has been minimized.

Copy link
@rite

rite Feb 11, 2025

Author Owner

Ideally, this is covered by the [have_recipe]-assertion above. Is this correct? If so, I'd like to get rid of these lines.

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

Yes!

This comment has been minimized.

Copy link
@rite

rite Feb 13, 2025

Author Owner

I'll provide a fixup commit.

assert {![have_board imx6q_sabrelite]}
assert {![have_board rpi3]}
assert {![have_board zynq_qemu]}
assert {![have_board zynq_usrp_e31x]}
assert {![have_board imx53_qsb_tz]}
assert {!([have_board imx53_qsb] && [have_spec foc])}

if {[get_cmd_switch --autopilot] && ([have_spec linux] ||
[have_board virt_qemu_riscv])} {
puts "\nAutopilot run is not supported on this platform\n"
exit 0
if {[get_cmd_switch --autopilot]} {
assert {![have_spec linux] && ![have_board virt_qemu_riscv]} \
Autopilot mode is not supported on this platform.
}

create_boot_directory
Expand Down
8 changes: 8 additions & 0 deletions repos/os/run/framebuffer.run
Original file line number Diff line number Diff line change
@@ -1,3 +1,7 @@
assert {[have_recipe pkg/[drivers_interactive_pkg]]} "
Recipe for 'pkg/[drivers_interactive_pkg]' not available.
"

create_boot_directory

import_from_depot [depot_user]/src/[base_src] \
Expand Down Expand Up @@ -56,4 +60,8 @@ install_config {

build_boot_image [build_artifacts]

assert {![have_cmd_arg --autopilot]} {
Autopilot mode is not supported by this run script.
}

run_genode_until forever
12 changes: 5 additions & 7 deletions repos/os/run/init_smp.run
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,12 @@ set demand [expr 1*1024*1024]
set cpus 4
set init_ram 256

if { [get_cmd_switch --autopilot] } {
if { [have_cmd_arg --autopilot] } {

if {[have_include "power_on/qemu"]} {
puts "\nRun script does not support Qemu.\n"
exit 0
}
assert {![have_include power_on/qemu]} \
Autopilot mode is not supported on this platform.

assert_spec nova
assert {[have_spec nova]}

This comment has been minimized.

Copy link
@rite

rite Feb 11, 2025

Author Owner

Is this run script supposed to be executed on base-hw as well?

This comment has been minimized.

Copy link
@nfeske

nfeske Feb 13, 2025

Let`s keep the existing condition for now, and look after your (valid) question afterwards.


# set specifically for our nightly test hardware */
if {[have_spec x86_64]} {
Expand Down Expand Up @@ -107,7 +105,7 @@ build_boot_image [build_artifacts]

append qemu_args " -m [expr 128 + $init_ram*$cpus]M -nographic -smp $cpus"

if { [get_cmd_switch --autopilot] } {
if { [have_cmd_arg --autopilot] } {

run_genode_until {bomb started} 20

Expand Down
4 changes: 2 additions & 2 deletions repos/os/run/internet_checksum.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec linux
assert {[have_spec linux]}

set tshark [installed_command tshark]
set trafgen [installed_command trafgen]
Expand All @@ -11,7 +11,7 @@ rename exit run_tool_exit
proc exit {{code 0}} {
global lx_fs_dir
global input_file
if {[get_cmd_switch --autopilot]} { exec rm -rf $input_file $lx_fs_dir }
if {[have_cmd_arg --autopilot]} { exec rm -rf $input_file $lx_fs_dir }
run_tool_exit $code
}
build { core init lib/ld lib/vfs test/internet_checksum server/lx_fs }
Expand Down
16 changes: 7 additions & 9 deletions repos/os/run/log_core.run
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
if {[have_spec linux]} {
puts "\n Run script is not supported on this platform. \n";
exit 0
}
if {[get_cmd_switch --autopilot] && ![have_include "power_on/qemu"]} {
puts "\n Run script is not supported on this platform. \n";
exit 0
assert {![have_spec linux]}

if {[have_cmd_arg --autopilot]} {
assert {[have_include power_on/qemu]} \
Autopilot mode is not supported on this platform.
}

proc log_service { } {
if { [get_cmd_switch --autopilot] } { return log }
if { [have_cmd_arg --autopilot] } { return log }
return ram
}

Expand Down Expand Up @@ -112,7 +110,7 @@ build_boot_image [build_artifacts]

append qemu_args " -nographic "

if { [get_cmd_switch --autopilot] } {
if { [have_cmd_arg --autopilot] } {
run_genode_until {.*\[log_core -> log] \[.*\n} 20
} else {
run_genode_until forever
Expand Down
2 changes: 1 addition & 1 deletion repos/os/run/lx_fs_import.run
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ lappend depot_archives [depot_user]/src/vfs_import

set build_components { }

if { [get_cmd_switch --autopilot] } {
if { [have_cmd_arg --autopilot] } {
lappend depot_archives [depot_user]/src/lx_fs
} else {
lappend build_components server/lx_fs
Expand Down
9 changes: 5 additions & 4 deletions repos/os/run/mixer.run
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
if {[have_include "power_on/qemu"]} {
puts "\nTest running on Qemu is not supported.\n"
exit 0
}
assert {![have_include power_on/qemu]}

build {
core init timer lib/ld
Expand Down Expand Up @@ -200,6 +197,10 @@ install_config {
</config>
}

assert {![have_cmd_arg --autopilot]} {
Autopilot mode is not supported by this run script.
}

if {[expr ![file exists bin/client1.f32] || ![file exists bin/client2.f32]]} {
puts ""
puts "The sample files are missing. Please take a look at repos/dde_bsd/README"
Expand Down
5 changes: 1 addition & 4 deletions repos/os/run/monitor.run
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@ proc platform_supported { } {
return 0
}

if {![platform_supported]} {
puts "Run script is not supported on this platform"
exit 0
}
assert {[platform_supported]}

build {
core lib/ld init timer monitor lib/sandbox
Expand Down
5 changes: 1 addition & 4 deletions repos/os/run/monitor_gdb.inc
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ proc platform_supported { } {
return 0
}

if {![platform_supported]} {
puts "Run script is not supported on this platform"
exit 0
}
assert {[platform_supported]}

create_boot_directory

Expand Down
9 changes: 4 additions & 5 deletions repos/os/run/nvme.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec x86
assert {[have_spec x86]}

# perform write tests when requested
if {[info exists env(GENODE_TEST_WRITE)]} {
Expand All @@ -14,10 +14,9 @@ set is_32bit_x86_hw [expr !$is_qemu && [have_spec 32bit]]
#
# Only run tests on supported platforms
#
if {[expr [have_spec linux] || $is_32bit_x86_hw || [expr $is_qemu && $is_old]]} {
puts "This run script is not supported on this platform."
exit 0
}
assert {![have_spec linux]}
assert {!$is_32bit_x86_hw}
assert {!($is_qemu && $is_old)}

#
# Qemu and on certain platforms only use the small set of tests
Expand Down
12 changes: 4 additions & 8 deletions repos/os/run/ping.run
Original file line number Diff line number Diff line change
Expand Up @@ -38,16 +38,12 @@ set on_linux_with_dst_ip ""
catch {set on_linux_with_dst_ip $::env(ON_LINUX_WITH_DST_IP)}
set on_linux [expr ![string equal $on_linux_with_dst_ip ""]]

if {[expr [have_spec linux] && !$on_linux]} {

puts "You must set 'on_linux_with_dst_ip' in the run script if you want to run on Linux."
exit 0
assert {!([have_spec linux] && !$on_linux)} {
You must set 'on_linux_with_dst_ip' in the run script if you want to run on Linux.
}

if {[expr ![have_spec linux] && $on_linux]} {

puts "You must run on Linux if 'on_linux_with_dst_ip' in the run script is set."
exit 0
assert {!(![have_spec linux] && $on_linux)} {
You must run on Linux if 'on_linux_with_dst_ip' in the run script is set.
}

set on_hardware [expr ![have_include power_on/qemu]]
Expand Down
5 changes: 1 addition & 4 deletions repos/os/run/platform.run
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
if { ![have_board pbxa9] } {
puts "Platform is unsupported."
exit 0
}
assert {[have_board pbxa9]}

create_boot_directory

Expand Down
6 changes: 5 additions & 1 deletion repos/os/run/pointer.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec linux
assert {[have_spec linux]}

build {
core init timer lib/ld
Expand Down Expand Up @@ -264,6 +264,10 @@ install_config {

build_boot_image [build_artifacts]

assert {![have_cmd_arg --autopilot]} {
Autopilot mode is not supported by this run script.
}

run_genode_until forever

# vi: set ft=tcl :
2 changes: 1 addition & 1 deletion repos/os/run/rom_to_file.run
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
assert_spec linux
assert {[have_spec linux]}

build { core init timer lib/ld server/dynamic_rom app/rom_to_file server/lx_fs }

Expand Down
5 changes: 1 addition & 4 deletions repos/os/run/sd_card.run
Original file line number Diff line number Diff line change
@@ -1,7 +1,4 @@
if {![have_board pbxa9]} {
puts "Test requires board pbxa9"
exit
}
assert {[have_board pbxa9]}

create_boot_directory
import_from_depot [depot_user]/src/[base_src] \
Expand Down
Loading

0 comments on commit e19c503

Please sign in to comment.