Skip to content
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.

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

add empty go.mod to avoid go mod problems #1038

Closed
wants to merge 1 commit into from
Closed

Conversation

gfr10598
Copy link
Contributor

@gfr10598 gfr10598 commented Dec 14, 2021

When building etl-gardener on darwin, go mod fails because of colons in paths in etl/parser/testdata. This allows go mod to ignore that directory.


This change is Reviewable

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7031

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 4 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.07%) to 64.15%

Files with Coverage Reduction New Missed Lines %
active/active.go 4 88.54%
Totals Coverage Status
Change from base Build 7029: -0.07%
Covered Lines: 3840
Relevant Lines: 5986

💛 - Coveralls

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

This feels a little like a hack, right? Is there a way to remove those files instead? IIRC files like that were also causing issues for App Engine builds at one time. It's an anti-pattern to allow.

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

IIUC, those are filenames within actual web100 and pt archives. I checked, and there are two tar.gz files in the testdata, and they both contain filenames with colons.

I believe that it is actually correct to ignore tests altogether in go mod. So I'm inclined to think we should add empty go.mod files to all testdata directories.

I guess you could consider it a hack that I develop on Darwin, but it is a terrific convenience, and I think it is worth enabling.

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor

@stephen-soltesz stephen-soltesz left a comment

Choose a reason for hiding this comment

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

Yes, I think I'm the one that created those tgz files b/c they used to be just plain files but the colons were causing trouble in App Engine. I've not seen the "empty go mod" pattern before. Something doesn't feel right.

Are these new files? Or, files that are not being cleaned up by https://github.com/m-lab/etl/blob/master/parser/parser_test.go#L218 ? Why aren't they being cleaned up? If our unit tests are doing something that is making our workflow harder, I think the solution is to fix the unit tests, e.g. unpack the files in a system /tmp directory or other out-of-bounds location. Do I have the full story?

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

I think go mod is now looking at the contents of the tar.gz files. Don't know why. I think this hasn't shown up before because we weren't using go mod, and when we started using go mod, I didn't try building on Darwin until today.

Looks like it hasn't made it into official documentation. There is this open issue: golang/go#31137

Reviewable status: 0 of 1 approvals obtained

Copy link
Contributor Author

@gfr10598 gfr10598 left a comment

Choose a reason for hiding this comment

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

The trouble starts for me when I try to run tests for gardener on Darwin...
go test ./job-service/...
go: cannot find main module, but found .git/config in /Users/gfr/go/src/github.com/m-lab/etl-gardener
to create a module there, run:
go mod init

Not sure why it wants me to run go mod, but when I do, I run into the issues with etl/parser/testdata. I guess if we can solve the "cannot find main module" problem, we can sidestep this for now, but it still seems to me like the empty go.mod file is an appropriate longterm solution.

Reviewable status: 0 of 1 approvals obtained

@gfr10598
Copy link
Contributor Author

Ok - my local problem was triggered by upgrading (some months ago) to go116. This version defaults GO111MODULE to "on" instead of "auto", so that triggered complaints that there was no main module, and one should run go mod init etc.

There is a single dependency in etl-gardener on etl/etl/globals.go, so go mod tidy includes etl.

So, this might be the fun bit. The tests include these lines:
./parser/parser_test.go: pipe.Exec("tar", "-C", "testdata", "-xvf", "testdata/pt-files.tar.gz"),
./parser/parser_test.go: pipe.Exec("tar", "-C", "testdata", "-xvf", "testdata/web100-files.tar.gz"),
./parser/parser_test.go: pipe.Exec("tar", "-C", "testdata", "-xvf", "testdata/sidestream-files.tar.gz"),
Perhaps go mod tidy is actually looking at these files because they are referenced in the go code.

UPSHOT:

  1. I can solve my local problem with GO111MODULE=auto.
  2. I think it is time to break the etl-gardener -> etl dependency, by moving common code into another repository that contains only the minimum code that is referenced by multiple repositories.
  3. We should probably pay more attention to any warning associated with go modules. I don't believe we should remove colons from filenames, just because some old operating systems made them reserved. If empty go.mod files in test directories avoid go mod problems, that seems like a reasonable solution. Avoiding or hiding code dependencies on archive files seems like the wrong solution.

@stephen-soltesz
Copy link
Contributor

Closing to clean up old PRs. For point 2 - #1083

@stephen-soltesz stephen-soltesz deleted the gfr-empty-go-mod branch August 12, 2022 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants