project-everest / everest-ci

CI scripts for project everest
3 stars 8 forks source link

replace `git clean -ffdx` with `everest clean` #38

Closed sishtiaq closed 5 years ago

sishtiaq commented 7 years ago

No CI script or action seems ,to make a call to ".\everest clean". The clean function currently fails as it try to call "cd Spartan/tools/Spartan && nmake clean" but there's no makefile in that directory. (I just commented it out at https://github.com/project-everest/everest/commit/b7298aaacadbd4771211382d29d53912f40469e7.)

This issue is not about Spartan vs. VALE or about make vs. scons. It's about lack of coverage for the clean function. A call to "./everest clean" from any place that ./everest is called in everest-ci/ci would fix this lack of coverage.

msprotz commented 7 years ago

Thanks for noticing this, and thanks for pushing a fix! However, I think we can actually fix the failing invocation rather than commenting it out.

Spartan currently uses scons to build, as you can see from the numerous occurrences in the script and in particular at https://github.com/project-everest/everest/blob/master/everest#L512 ; running scons --help tells me:

  -c, --clean, --remove       Remove specified targets and dependencies.

So, what needs to happen to properly fix this is:

# $1: arguments to scons
function run_scons() {
  # re-use the code that defers to CMD or directly scons, and pass $1 to scons
}

then:

build_spartan() {
  run_scons
}

and:

do_clean() {
  run_scons "--clean"

This is, naturally, pseudo-code, but I believe it's relatively easy to fix. Do you want to take this? I'd be happy to review a pull request for that.

Thanks,

Jonathan

sishtiaq commented 7 years ago

Work done on https://github.com/project-everest/everest/pull/16.

"everest-ci" still needs to call "everest clean".

msprotz commented 7 years ago

Yes; this is the line that needs to change https://github.com/project-everest/everest-ci/blob/master/ci#L253

sishtiaq commented 7 years ago

Here?

sishtiaq@SaminDesktop /cygdrive/e/everest-ci
$ git diff ci
diff --git a/ci b/ci
index 5fd6436..880a945 100755
--- a/ci
+++ b/ci
@@ -251,7 +251,7 @@ refresh_fstar_hints () {

 everest_rebuild () {
   git clean -ffdx
-  ./everest --yes check reset make
+  ./everest --yes check reset clean make
 }

And no where else?

msprotz commented 7 years ago

Yes, and then you can remove the git clean line.

This relies on the assumption that everest clean removes everything needed to start afresh... which I haven't verified :). So go ahead, but be aware that we may have build failures and may need to followup.