-
Notifications
You must be signed in to change notification settings - Fork 17
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
Conversation
FYI @dirkmc |
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. |
There was a problem hiding this 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.
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 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 I'm not attached to the name
I wouldn't say so, in general. It should be relatively rare for repositories to need multiple modules. |
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 |
Undone the move to @masih I'm leaving the makefile and dockerfile to you for a follow-up PR, since you volunteered :) |
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.
Hmm, how many times can one get |
There was a problem hiding this 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.
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 |
Since the git repository and Go module were renamed in ipni/index-provider#93.
Since the git repository and Go module were renamed in ipni/index-provider#93.
(see commit message)