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

split cmd/ into a separate module #93

Merged
merged 1 commit into from Nov 16, 2021
Merged

split cmd/ into a separate module #93

merged 1 commit into from Nov 16, 2021

Conversation

mvdan
Copy link
Contributor

@mvdan mvdan commented Nov 15, 2021

(see commit message)

@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

FYI @dirkmc

@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

CI should still work, thanks to the multiple-go-modules action - it should still be testing all modules.

It looks like I messed up the placement of some testdata files, though.

Copy link
Collaborator

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

If this project is primarily a lib, it seems like a /lib directory is superfluous. I suggest considering that every exported package live in its own directory under root. Things like cmd/ will be recognized to be special.

Either way, this is an important consideration since ti establishes repo organizational patterns that will be duplicated in other repos.

cmd/provider/daemon.go Outdated Show resolved Hide resolved
@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

If this project is primarily a lib, it seems like a /lib directory is superfluous.

From a conceptual level it's certainly unnecessary, but from a practical level I want to avoid overlapping Go modules. That is, if we have /go.mod and /cmd/go.mod, you end up with "nested Go modules", which make using them more confusing. For example, if someone does a go get of [...]/index-provider/cmd/provider, that's an ambiguous import path; the Go tool must check if either the index-provider or index-provider/cmd modules provide such a package. It could be none of them (causing an error), just one of them, or both of them (also causing an error).

Another example where we can cause user confusion is that the module zip for the root module won't include the nested module directory. This is fairly obvious if the two modules aren't overlapping, but if they are, you could be forgiven for being bitten by this detail.

We could certainly avoid the /lib prefix and use nested Go modules, it's not impossible. It would add sharp edges, though. One useful data point is that in go-car we did end up with /go.mod and /cmd/go.mod, but note that the actual latest library is under /v2/go.mod, so that still doesn't overlap with /cmd.

I'm not attached to the name /lib either. If we want to keep the non-overlapping modules but use a different name, that's fine too.

Either way, this is an important consideration since ti establishes repo organizational patterns that will be duplicated in other repos.

I wouldn't say so, in general. It should be relatively rare for repositories to need multiple modules.

@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

briefly discussed in colo - since we've used overlapping/nested modules in other repos like go-car and go-ipld-prime, and they haven't caused much fuss, we're going to be practical and drop the /lib prefix.

@mvdan mvdan changed the title split module into cmd/ and lib/ split cmd/ into a separate module Nov 15, 2021
@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

Undone the move to lib/. Should be ready to merge now.

@masih I'm leaving the makefile and dockerfile to you for a follow-up PR, since you volunteered :)

@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

Huh, I'm really not having a good day today with Go master. Found another bug while working on this PR. golang/go#49598

Ignite want to reuse some of the library packages in this module,
such as the engine package.
However, they can't require the overall module,
as cmd/provider pulls heavy dependencies like go-ipfs.

Split the module into two; one for the actual daemon we can run,
and another for the set of library packagse others may reuse.

Note this also required moving some library packages out of "internal",
as otherwise cmd/provider can't reasonably import internal/foo.

Also note that we introduce a dependency bump dance;
whenever cmd needs to update the root library's version,
we need to first push one version of the library to git,
and then "go get" that pushed version into cmd.
For now, we've done that via the module-split branch.

Once Go 1.18 releases, we'll be able to add a root go.work file to
simplify the development of the two modules at the same time.
Note that we'll still need to do the "git push" and "go get" dance
if we want cmd/provider to be useful as a standalone module.

While here, rename "package suppliers" to "supplier",
as per Andrew's suggestion.
@mvdan
Copy link
Contributor Author

mvdan commented Nov 15, 2021

Hmm, how many times can one get go mod tidy and gofmt wrong? Apologies for the noise. Fingers crossed it's green now.

go.mod Show resolved Hide resolved
Copy link
Collaborator

@gammazero gammazero left a comment

Choose a reason for hiding this comment

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

I would prefer to also git rid of the cmd/priovider/ directory and put everything there into cmd/. That can be done in a separate PR if preferable.

@mvdan
Copy link
Contributor Author

mvdan commented Nov 16, 2021

I would prefer to also git rid of the cmd/priovider/ directory and put everything there into cmd/. That can be done in a separate PR if preferable.

I'd keep the sub-folder, because it's possible we will have other commands later on. It's also nice when one can do go install ./cmd/provider and the binary name ends up being provider; otherwise one ends up with a binary named cmd by default.

@mvdan mvdan merged commit a46181c into ipni:main Nov 16, 2021
mvdan added a commit to ipni/storetheindex that referenced this pull request Nov 16, 2021
Since the git repository and Go module were renamed in
ipni/index-provider#93.
mvdan added a commit to ipni/storetheindex that referenced this pull request Nov 16, 2021
Since the git repository and Go module were renamed in
ipni/index-provider#93.
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.

None yet

3 participants