elves / elvish

Powerful scripting language & versatile interactive shell
https://elv.sh/
BSD 2-Clause "Simplified" License
5.52k stars 296 forks source link

The CI only runs lint checks (`make all-checks`) on Ubuntu #1670

Open krader1961 opened 1 year ago

krader1961 commented 1 year ago

I recently rewrote my script that automates tasks, such as building Elvish and running its unit-tests, on all of my systems from Bash to Elvish. That made it much easier to add another task: running make all-checks on each system. That revealed an unexpected error:

Linting Elvish...
macpro      2.767 seconds (1.0x)
fedora37    3.113 seconds (1.1x)
fedora28   13.886 seconds (4.5x)
osuse      14.110 seconds (1.0x)
ubuntu32   14.456 seconds (1.0x)
freebsd    14.564 seconds (1.0x)
ubuntu64   15.015 seconds (1.0x)
msys       24.078 seconds (1.6x)
  ./tools/check-fmt-go.sh
  Go files need these changes:
    None!
  ./tools/check-fmt-md.sh
  Markdown files that need changes:
    None!
  ./tools/check-disallowed.sh
  codespell
  go vet ./...
  staticcheck ./...
  pkg\sys\eunix\termios_32bitflag.go:5:6: type termiosFlag is unused (U1000)
  make: *** [Makefile:41: most-checks] Error 1
  Exception: make exited with 2
  [tty 17]:1:1: make all-checks

That error isn't being caught by the CI (continuous integration) environment because it only runs make all-checks on Ubuntu and not Windows (or another non-Linux OS). I'll open a pull-request that fixes the cause of that error. This issue is to discuss whether it makes sense to modify the CI config to run make all-checks on every OS target given the additional cost of doing so.

P.S., Fixing the build constraints reveals another problem. A generated file only used on Windows is out of date:

Linting Elvish...
macpro      2.868 seconds (1.0x)
fedora37    3.195 seconds (1.1x)
fedora28   13.189 seconds (4.1x)
osuse      13.527 seconds (1.0x)
ubuntu32   13.750 seconds (1.0x)
freebsd    13.973 seconds (1.0x)
ubuntu64   14.191 seconds (1.0x)
msys       40.497 seconds (2.9x)
  ./tools/check-fmt-go.sh
  Go files need these changes:
    None!
  ./tools/check-fmt-md.sh
  Markdown files that need changes:
    None!
  ./tools/check-disallowed.sh
  codespell
  go vet ./...
  staticcheck ./...
  ./tools/check-gen.sh
  ======================================================================
  Generated Go code is out of date. See
  https://github.com/elves/elvish/blob/master/CONTRIBUTING.md#generated-code
  ======================================================================
   M pkg/sys/ewindows/ztypes_windows.go
  make: *** [Makefile:44: all-checks] Error 1
  Exception: make exited with 2
  [tty 25]:1:1: make all-checks
krader1961 commented 1 year ago

The reason for the ./tools/check-gen.sh failure is because the generated file, pkg/sys/ewindows/ztypes_windows.go, contains this line:

// cgo.exe -godefs C:\Users\xiaq\on\elvish\pkg\sys\ewindows\types.go

It should apparently contain this line:

// cgo.exe -godefs types.go
xiaq commented 1 year ago

It is not necessary to run go vet or staticcheck on multiple platforms. Setting GOOS and GOARCH is sufficient.

The check-gen script does need to be run on multiple platforms.

The other checks do not depend on GOOS or GOARCH.

krader1961 commented 1 year ago

Playing devil's advocate: It still seems the simplest, easiest to understand and maintain, option is to simply run make all-checks on every platform in the CI test matrix rather than just Ubuntu. Doing so should be cheap and protects against future changes that might invalidate the current assumptions vis-a-vis make most-checks being platform independent. On the other hand having to install codespell and other tools on every CI platform to run checks that are platform independent is wasteful. Which argues for modifying the CI config to run ./tools/check-gen.sh on every platform in the CI matrix and modifying the "checks:" CI job to run make most-checks rather than make all-checks. That is more efficient but also more fragile (at least in theory).

I am ambivalent about which approach is preferable. However, one of the changes proposed above is needed because the CI environments should catch any error I would find running the same commands on my local systems.