CI: mac os x linter warnings ignored

[kolyshkin]

I have (accudentally) noticed that on Mac OS X the warnings from golangci-lint are ignored. For example, see
https://github.com/kolyshkin/sys/runs/4823094862?check_suite_focus=true

This can be caused by shell differences and the use of subshell by Makefile here:

sys/Makefile

Line 19 in 03b9f8d

(cd $$p && go test $(RUN_VIA_SUDO) -v .); \

Alas I don't have a Mac to debug this. @thaJeztah maybe you could take a look?

[kolyshkin]

Sorry, I meant this place:

sys/Makefile

Lines 37 to 38 in 03b9f8d

	(cd $$p && go mod download \
	&& ../$(BINDIR)/golangci-lint run); \

I am 95% sure golangci-lint exits with a non-zero code here, and yet it's not returned by make.

[thaJeztah]

Not at my computer right now, but reminds me of #19, which was meant to fix something similar?

[thaJeztah]

So it looks to fail on my machine (at commit 03b9f8d), so not sure why it wouldn't fail in CI 🤔

make lint
_build/bin/golangci-lint version
golangci-lint has version 1.43.0 built from 861262b7 on 2021-11-03T11:57:46Z
for p in mountinfo mount signal symlink; do \
		(cd $p && go mod download \
		&& ../_build/bin/golangci-lint run); \
	done
ERRO Running error: context loading failed: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /Users/sebastiaan/go/src/github.com/moby/sys/mountinfo:
	golang.org/x/sys@v0.0.0-20211025201205-69cdffdb9359: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

	To ignore the vendor directory, use -mod=readonly or -mod=mod.
	To sync the vendor directory, run:
		go mod vendor
ERRO Running error: context loading failed: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /Users/sebastiaan/go/src/github.com/moby/sys/mount:
	github.com/moby/sys/mountinfo@v0.5.0: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt
	golang.org/x/sys@v0.0.0-20211025201205-69cdffdb9359: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

	To ignore the vendor directory, use -mod=readonly or -mod=mod.
	To sync the vendor directory, run:
		go mod vendor
ERRO Running error: context loading failed: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /Users/sebastiaan/go/src/github.com/moby/sys/signal:
	golang.org/x/sys@v0.0.0-20211025201205-69cdffdb9359: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

	To ignore the vendor directory, use -mod=readonly or -mod=mod.
	To sync the vendor directory, run:
		go mod vendor
ERRO Running error: context loading failed: failed to load packages: failed to load with go/packages: err: exit status 1: stderr: go: inconsistent vendoring in /Users/sebastiaan/go/src/github.com/moby/sys/symlink:
	golang.org/x/sys@v0.0.0-20211025201205-69cdffdb9359: is explicitly required in go.mod, but not marked as explicit in vendor/modules.txt

	To ignore the vendor directory, use -mod=readonly or -mod=mod.
	To sync the vendor directory, run:
		go mod vendor
make: *** [lint] Error 3
echo $?
2

[thaJeztah]

argh, never mind; looks like I had some vendor directory around for testing; let me check again

[thaJeztah]

Yes, I can reproduce; getting a zero exit code indeed;

make lint
_build/bin/golangci-lint version
golangci-lint has version 1.43.0 built from 861262b7 on 2021-11-03T11:57:46Z
for p in mountinfo mount signal symlink; do \
		(cd $p && go mod download \
		&& ../_build/bin/golangci-lint run); \
	done
mounter_unsupported.go:6:6: `mount` is unused (deadcode)
func mount(device, target, mType string, flag uintptr, data string) error {
     ^
mount_errors.go:45:22: func `(*mountError).Unwrap` is unused (unused)
func (e *mountError) Unwrap() error {
                     ^
mount_errors.go:9:6: type `mountError` is unused (unused)
type mountError struct {
     ^
mount_errors.go:17:22: func `(*mountError).Error` is unused (unused)
func (e *mountError) Error() string {
                     ^
mount_errors.go:39:22: func `(*mountError).Cause` is unused (unused)
func (e *mountError) Cause() error {
                     ^
echo $?
0

[kolyshkin]

@thaJeztah I guess you can run some tool to trace the system calls and see where non-zero exit code is ignored. On Linux, I can use something like strace -f -o out make lint.

Alternatively, run make -d lint and see what make has to say about exit codes.

Alternatively, start with something simple in the Makefile like

.PHONY: fail
fail:
	false

(so make fail should error out)

then do

.PHONY: fail
fail:
	( false )

(same but running in a subshell)

then

.PHONY: fail
fail:
	for x in 1 2 3; do \
		(echo $$x && false ); \
	done

etc. IOW make the code more and more resembling the lint target.

Overall I think this is due to differences in either make or /bin/sh.

Once you figure out why exactly it fails, I hope we can rewrite it.

Also summoning @tianon -- not sure if you have any Mac OS X experience, but just in case you do, PTAL 🙏🏻

[kolyshkin]

To clarify -- all the examples from the comment above result in

$ make fail
...
make: *** [Makefile:14: fail] Error 1

on Linux (GNU make / bash).

[kolyshkin]

on Linux (GNU make / bash).

As well as with dash and busybox sh (e.g. make SHELL="busybox sh" fail).

[thaJeztah]

So, I tested with this Makefile:

.SHELLFLAGS = -ec

.PHONY: fail1
fail1:
        false

.PHONY: fail2
fail2:
        ( false )

.PHONY: fail3
fail3:
        for x in 1 2 3; do \
                (echo $$x && false ); \
        done

Using:

make --version
GNU Make 3.81
Copyright (C) 2006  Free Software Foundation, Inc.
This is free software; see the source for copying conditions.
There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A
PARTICULAR PURPOSE.

And all three of them fail:

make fail1
false
make: *** [fail1] Error 1

make fail2
( false )
make: *** [fail2] Error 1

make fail3
for x in 1 2 3; do \
		(echo $x && false ); \
	done
1
2
3
make: *** [fail3] Error 1

So still not sure immediately why the make lint case would not fail on the same 🤔

[kolyshkin]

What about this one?

.SHELLFLAGS = -ec

.PHONY: fail
fail:
	for x in 1 2 3; do \
		(echo $$x && [ $$x -gt 1 ]); \
	done

The idea here is to repro the actual fail (it fails in 1 and passes in 2 and 3, same as the real test in description)/

On my machine it passes without SHELLFLAGS = -ec, and fails it (as it should).

[thaJeztah]

Actually; perhaps I should try if it only fails if the last one fails, so something like this would then "pass"

for x in 2 1 0; do \
  (echo $$x && exit $$x); \
done

[kolyshkin]

So, this is the code that should fail on all platforms and yet it does not fail on Mac.

#104

[tianon]

export SHELL := bash

😇

More seriously, is there any way we can avoid the cd? Perhaps pushd+popd so we can skip the subshell completely?

Also, you probably want to include set -e; on all those for statement lines - I do not believe Make includes it automatically, ie:

lint: $(BINDIR)/golangci-lint
	$(BINDIR)/golangci-lint version
	set -eux; for p in $(PACKAGES); do \
		(cd $$p && go mod download \
		&& ../$(BINDIR)/golangci-lint run); \
	done

(Using ../$(BINDIR)/... also feels a little sketchy if users do something like make BINDIR=/absolute/path but probably not important for lint 😄)