search

seL4 / sel4test

Test suite for seL4.
http://sel4.systems
Other
25 stars 63 forks source link

add conditional subbranch for the flag SIMULATION #73

Closed ichdream closed 2 years ago

ichdream commented 2 years ago

Then, the cmake tool will not execute the codes within if condition when SIMULATION is OFF. And the switch is need to be opened by user. Since, no matter the flag SIMULATION is ON or OFF, there always exists a simulate program on the build floder. It confuses us, and after fix it, it looks clean.

Signed-off-by: ichdream chensheng0802@outlook.com

axel-h commented 2 years ago

Thanks, you have a point there that always doing this could be confusing - unless you just accept this. Making this conditional seems a nice thing, but I wonder how many worksflows it break, that SIMULATION must be set manually now.

ichdream commented 2 years ago

Thanks, you have a point there that always doing this could be confusing - unless you just accept this. Making this conditional seems a nice thing, but I wonder how many worksflows it break, that SIMULATION must be set manually now.

SIMULATION is set off by default, if the user does not set it on explicitly. We had the duty to tell users that if you want simulation, you should switch the flag on, then you will get the simulate program. As to the workflows it break and must set manually, i find it seems do little. For example, we build the seL4test for the rockpro64 board, you should set the SIMULATION manually on.

../init-build.sh -DPLATFORM=x86_64 -DSIMULATION=TRUE

After modify it, the build flow does not change any.

ichdream commented 2 years ago

Thanks, you have a point there that always doing this could be confusing - unless you just accept this. Making this conditional seems a nice thing, but I wonder how many worksflows it break, that SIMULATION must be set manually now.

SIMULATION is set off by default, if the user does not set it on explicitly. We had the duty to tell users that if you want simulation, you should switch the flag on, then you will get the simulate program. As to the workflows it break and must set manually, i find it seems do little. For example, we build the seL4test for the rockpro64 board, you should set the SIMULATION manually on.

../init-build.sh -DPLATFORM=x86_64 -DSIMULATION=TRUE

After modify it, the build flow does not change any.

Since the flag SIMULATION if set OFF by default, that means user do not set it explicitly. Why it generates the simulate program ? The generated simulate program can not run normally.So we do not need it. If the user need it, he or she may set it manually, as the tutorial teach us. https://docs.sel4.systems/GettingStarted

axel-h commented 2 years ago

From my side this change seems ok. A bit of polishing for the commit message:

  cmake: create script only if SIMULATION is enabled

  The simulation scripts are useless if the target is a real board and
  not a QEMU simulation. Thus they will only be created if SIMULATION
  is explicitly ON (-DSIMULATION=TRUE).
ichdream commented 2 years ago

From my side this change seems ok. A bit of polishing for the commit message:

  cmake: create script only if SIMULATION is enabled

  The simulation scripts are useless if the target is a real board and
  not a QEMU simulation. Thus they will only be created if SIMULATION
  is explicitly ON (-DSIMULATION=TRUE).

It looks like better, thanks.

axel-h commented 2 years ago

It looks like better, thanks.

Glad you are ok with this. You still have to make the change, though. Then the failing CI check should also pass and we can merge this.

ichdream commented 2 years ago

make

It looks like better, thanks.

Glad you are ok with this. You still have to make the change, though. Then the failing CI check should also pass and we can merge this.

okay， i had done it.

ichdream commented 2 years ago

make

It looks like better, thanks.

Glad you are ok with this. You still have to make the change, though. Then the failing CI check should also pass and we can merge this.

okay， i had done it.

it seems to be blocked in one process, i am not sure...

axel-h commented 2 years ago

"The switch is set off by default, and need to be opened by user." sounds a bit wired, should probably simply say "SIMULATION is disabled by default"

axel-h commented 2 years ago

it seems to be blocked in one process, i am not sure...

The CI run needs to be started manually for any users submitting a PR for the first time.

ichdream commented 2 years ago

"The switch is set off by default, and need to be opened by user." sounds a bit wired, should probably simply say "SIMULATION is disabled by default"

Indeed, it is more streamlined. I will modify it and try it again.

axel-h commented 2 years ago

Are you dropping this PR?

lsf37 commented 2 years ago

This looks like a force-push accident. @ichdream did you mean to close the PR?

ichdream commented 2 years ago

This looks like a force-push accident. @ichdream did you mean to close the PR?

Are you dropping this PR?

Sorry to confuse, i just want to re-submmit a pr, since the original one is not perfect, and will do it later.