Skip to content

mount: also test against local mountinfo source code #68

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
thaJeztah wants to merge 1 commit into from
Closed

mount: also test against local mountinfo source code #68

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/mount/go-local.*
20 changes: 18 additions & 2 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,31 @@ CROSS ?= linux/arm linux/arm64 linux/ppc64le linux/s390x \
SUDO ?= sudo -n

.PHONY: all
all: lint test cross
all: clean lint test test-local cross
Copy link
Collaborator

@kolyshkin kolyshkin Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: no need to have test-local here as test depends on it.


.PHONY: clean
clean:
$(RM) mount/go-local.*

.PHONY: test
test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
Copy link
Collaborator

@kolyshkin kolyshkin May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would change this to

Suggested change
test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

(and move up) to avoid repetition.

Copy link
Member Author

@thaJeztah thaJeztah Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(and move up) to avoid repetition.

You mean, move test-local above test ?

Copy link
Collaborator

@kolyshkin kolyshkin Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean, combine the two lines

test: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
....
test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

into one

test test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")

and move it up next to SUDO definition.

test:
test: test-local
for p in $(PACKAGES); do \
(cd $$p && go test $(RUN_VIA_SUDO) -v .); \
done

# test the mount module against the local mountinfo source code instead of the
# release specified in go.mod. This allows catching regressions / breaking
# changes in mountinfo.
.PHONY: test-local
test-local: RUN_VIA_SUDO = $(shell $(SUDO) true && echo -exec \"$(SUDO)\")
test-local:
echo 'replace github.com/moby/sys/mountinfo => ../mountinfo' | cat mount/go.mod - > mount/go-local.mod
cd mount \
&& go mod download -modfile=go-local.mod \
&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .
Comment on lines +29 to +31
Copy link
Collaborator

@kolyshkin kolyshkin May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
cd mount \
&& go mod download -modfile=go-local.mod \
&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .
cp mount/go.sum mount/go-local.sum
cd mount && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

or

Suggested change
cd mount \
&& go mod download -modfile=go-local.mod \
&& go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .
ln -sf go.sum mount/go-local.sum
cd mount && go test -modfile=go-local.mod $(RUN_VIA_SUDO) -v .

Copy link
Member Author

@thaJeztah thaJeztah Sep 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if understand this change; go-local.sum will be different than go.sum, because it's using different versions of the dependency; the go mod download -modfile=go-local.mod may pull other versions of dependencies, because mountinfo ("main branch") may be using different versions of dependencies.

Copy link
Collaborator

@kolyshkin kolyshkin Oct 25, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took me a while to figure out why I originally suggested that :)

This is because

  1. the only difference between go.mod and go-local.mod is the replace directive we add;
  2. go mod download is not needed at all -- there is nothing to download, the module is here already;
  3. for the modules with relevant paths (aka local modules), no checksums are ever needed or recorded.

This means that existing go.sum file is totally adequate, so copying or linking it is sufficient. You can even try to do go mod tidy -modfile go-local.mod and see what happens -- in fact it will remove the checksums for mountinfo v0.4.1 package (as it is no longer used), but having this extra lines is not a problem for tests.

$(RM) mount/go-local.*

.PHONY: lint
lint: $(BINDIR)/golangci-lint
$(BINDIR)/golangci-lint version
Expand Down