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

all: add mod file #632

Merged
merged 1 commit into from
May 5, 2019
Merged

all: add mod file #632

merged 1 commit into from
May 5, 2019

Conversation

urandom2
Copy link
Contributor

@urandom2 urandom2 commented Apr 7, 2019

  • go modules will be enabled in 1.13.
  • after merge it would be nice if you would tag this repo: e.g. v1.1.17

- [go modules](https://github.com/golang/go/wiki/Modules) will be enabled in 1.13.
- after merge it would be nice if you would tag this repo: e.g. `v1.1.17`
@notti
Copy link
Collaborator

notti commented Apr 8, 2019

I haven't had the time to fully read up on modules so far. Is it possible to somehow split this into subdirectories? E.g. golang.org/x/net is only used by afpacket, and pcapgo. So if one uses pcap, pfring, or just layers, net is not needed.
Same for x/sys/unix (what happens on windows?).
Furthermore, what does go 1.12 mean. Is this the mod format? (I couldn't find this one with some quick googling).
If we fix those issues (if they are issues - might be that we can just ignore this?) it would also be nice to get #627 in before the next release - otherwise I'd have no issue to tag a new release after putting this into production (maybe try it out for a bit if it works before...)

@urandom2
Copy link
Contributor Author

urandom2 commented Apr 8, 2019

You can break things up into sub modules if you would like, but they are heirarchical, so you would need separate modules per sub-directory. Though you could put most everything under the root module and break off a few problematic packages as sub modules.

Usually submodules are used for versioning purposes: e.g. I want to break compatibility for net, but nothing else, thus net would have net/v2.0.0 and others would still be v1.1.20.

It is also worth noting that today, since you have no go.mod, that customers are depending on the pull repo, e.g. including net, and this dependency is just at build, and no extraneous packages are included in binaries.

From 1.12 release notes:

The go directive in a go.mod file now indicates the version of the language used by the files within that module. It will be set to the current release (go 1.12) if no existing version is present. If the go directive for a module specifies a version newer than the toolchain in use, the go command will attempt to build the packages regardless, and will note the mismatch only if that build fails.

@notti
Copy link
Collaborator

notti commented Apr 9, 2019

So I read up a bit on modules, compared with the pull request, and found out the following things:

  • x/sys/unix is not a problem since go.mod contains just x/sys
  • I think we can live with people having to download x/net, x/sys (and the dependencies x/crypto x/text)
  • if I understood correctly GOPATH approach continues to work since we don't have to change anything apart from adding the go.mod and go.sum, which get ignored in that case anyway.
  • (Nearly?) All our existing users are on GOPATH, so we definitely should integrate this in our CI test setup or things will go bad in the near future (e.g., pull requests and code reviews that don't find issues in go.mod).

go 1.12 line is ahhh interesting. According to golang/go#30791 we should put 1.6 there... But I'm not sure if anything will explode then (especially since since x/net already says go 1.12). This looks a bit unfinished.

So we definitely need to include CI testing if we include this and could try what happens if we change the go version.

@urandom2
Copy link
Contributor Author

  • if I understood correctly GOPATH approach continues to work since we don't have to change anything apart from adding the go.mod and go.sum, which get ignored in that case anyway.

Yes, this should not break any existing workflows.

go 1.12 line is ahhh interesting. According to golang/go#30791 we should put 1.6 there... But I'm not sure if anything will explode then (especially since since x/net already says go 1.12). This looks a bit unfinished.

From ianlancetaylor:
The basic guideline is fairly simple: a "go" directive 1.N means that the code in that module is permitted to use language features that existed in 1.N even if those features were removed in later releases of the language.

Based on the above comment from the linked ticket, I am led to believe that 1.12 is the correct choice. Either way, I have tested this with 1.6 on my local system and everything looks functional, so we could change it later.

So we definitely need to include CI testing if we include this and could try what happens if we change the go version.

sgtm; would you rather block this PR on CI, or is manual testing sufficient so module consumers can get the dependency info.

@urandom2
Copy link
Contributor Author

@notti, any followup? Happy to leave this sit if there are still decisions being made, but I wanted to close the loop if you are ready to merge.

@notti
Copy link
Collaborator

notti commented Apr 30, 2019

My plan is to fix the ci testing sometime this week and then merge this.

@notti notti merged commit 81f5b55 into google:master May 5, 2019
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.

2 participants