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

switch to go module #4116

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

switch to go module #4116

wants to merge 3 commits into from

Conversation

crazy-max
Copy link
Member

@crazy-max crazy-max commented Mar 24, 2023

follow-up #3387 (comment)

- What I did

Looks like we are ready to switch to go module for this repo as we are now compatible with Go's conventions (we are SemVer since v23.0.0).

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cc @thaJeztah @neersighted @corhere @vvoland @rumpl

@thaJeztah
Copy link
Member

thaJeztah commented Mar 24, 2023

We'll have to rename all import paths (to include v23 (or likely v24 - see below) in the name, and rename the module to be the same.

Given that master is targeting v24, we probably need to tag the branch with a v24.0.0-alpha.0, otherwise it won't be importable as go modules produces a pseudo version based on the last tag from master (which I think was a v23)

@crazy-max
Copy link
Member Author

We'll have to rename all import paths (to include v23 (or likely v24 - see below) in the name, and rename the module to be the same.

That's exactly what I missed 🙈

@codecov-commenter
Copy link

codecov-commenter commented Mar 31, 2023

Codecov Report

Merging #4116 (9e3954b) into master (33961a7) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4116   +/-   ##
=======================================
  Coverage   59.03%   59.03%           
=======================================
  Files         287      287           
  Lines       24767    24767           
=======================================
  Hits        14620    14620           
  Misses       9264     9264           
  Partials      883      883           

@crazy-max
Copy link
Member Author

crazy-max and others added 3 commits April 1, 2023 15:31
Looks like we are ready to switch to go module for this repo
as we are now compatible with Go's conventions (we are SemVer
since v23.0.0).

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
@thaJeztah
Copy link
Member

thaJeztah commented Apr 1, 2023

Thanks, looks hopeful (so far). FWIW; I was discussing this briefly in the maintainers call, and I think the initial response was to "Let's do both Moby and CLI at the same time". I'm personally partial to either (same time, or separately), but before we make the decision and plunge in, we should;

  • Do some testing / verification to see if there's other pain-points where we may be painting ourselves into a corner (more below)
  • Document the steps (and order / "sequence of steps") what to do when preparing a new release. The intent currently is to bump the "major" version fo every release, which means we will "rename the module" for every (non-patch) release

Reason this is important is that we have a very complex dependency tree due to circular dependencies;

  • Some of that we don't run into YET, because moby/moby is not a go module yet (so "has no version requirements for any of its dependencies")
    • ^^^ effectively, because docker/docker (moby) only has a vendor.mod (no go.mod), it currently "breaks the circular loop"
    • ^^^ once we have a go.mod in docker/docker (moby), that's no longer the case, and potentially "all hell breaks loose"
  • Some of that we may not run into ourselves but others may (thinking of docker compose, which depends on both docker/docker, docker/cli, AND docker/buildx)

A very quick (and limited) overview of this;

  • (1) docker/cli
    • (1.1) -> depends on docker/docker
      • (1.1.1) -> depends on buildkit (see 1.2)
      • (1.1.2) -> depends on containerd (see 1.4)
    • (1.2) -> depends on buildkit
      • (1.2.1) -> depends on docker/docker (back to 1.1)
      • (1.2.2) -> depends on docker/cli (back to 1)
      • (1.2.3) -> depends on containerd (see 1.4)
    • (1.3) -> depends on swarmkit
      • (1.2 A) -> depends on docker/docker (back to 1.1)
      • (1.2 B) -> depends on docker/cli (back to 1)
    • (1.4) -> depends on containerd
  • (2) docker/buildx
    • (2.1) -> depends on docker/cli (back to 1)
    • (2.2) -> depends on docker/docker (back to 1.1)
    • (2.3) -> depends on buildkit (back to 1.2)
    • (2.4) -> depends on containerd (see 1.4)
  • (3) docker/compose/v2
    • (3.1) -> depends on docker/cli (back to 1)
    • (3.2) -> depends on docker/buildx (back to 2)
    • (3.3) -> depends on buildkit (back to 1.2)
    • (3.4) -> depends on containerd (see 1.4)

Cut down circular dependencies

Perhaps we're able to remove docker/cli from some places.

  • I noticed that BuildKit only uses a very minimal set of packages, and perhaps those (or part of?) could be a separate module.
  • Possibly BuildKit's binaries (cmd/buildctl), which are probably not expected to be used as module, could be changed to a "sub" module
  • Possibly frontend/dockerfile could still be made its own module (and repo?)
  • Possibly builder/remotecontext/urlutil could still be made its own module (and repo?)

Looking at docker/cli's dependency on buildkit;

tree -d ./vendor/github.com/moby/buildkit
./vendor/github.com/moby/buildkit
├── frontend
│   └── dockerfile
│       └── dockerignore
└── util
    └── appcontext

Looking at buildkit, this is what it depends on;

tree -d ./vendor/github.com/docker/cli
./vendor/github.com/docker/cli
└── cli
    ├── config
    │   ├── configfile
    │   ├── credentials
    │   └── types
    └── connhelper
        └── commandconn

Slightly more from docker/docker (but those are expected, and "should be ok" for most of them);

./vendor/github.com/docker/docker
├── api
│   └── types
│       ├── blkiodev
│       ├── container
│       ├── events
│       ├── filters
│       ├── image
│       ├── mount
│       ├── network
│       ├── registry
│       ├── strslice
│       ├── swarm
│       │   └── runtime
│       ├── time
│       ├── versions
│       └── volume
├── client
├── errdefs
├── libnetwork
│   └── resolvconf
├── pkg
│   ├── archive
│   ├── chrootarchive
│   ├── homedir
│   ├── idtools
│   ├── ioutils
│   ├── longpath
│   ├── pools
│   ├── reexec
│   └── system
└── profiles
    └── seccomp

For buildx;

tree -d ./vendor/github.com/docker/cli
./vendor/github.com/docker/cli
├── cli
│   ├── command
│   ├── config
│   │   ├── configfile
│   │   ├── credentials
│   │   └── types
│   ├── connhelper
│   │   ├── commandconn
│   │   └── ssh
│   ├── context
│   │   ├── docker
│   │   └── store
│   ├── debug
│   ├── flags
│   ├── manifest
│   │   ├── store
│   │   └── types
│   ├── registry
│   │   └── client
│   ├── streams
│   ├── trust
│   └── version
├── cli-plugins
│   ├── manager
│   └── plugin
└── opts
tree -d ./vendor/github.com/docker/docker
./vendor/github.com/docker/docker
├── api
│   └── types
│       ├── blkiodev
│       ├── container
│       ├── events
│       ├── filters
│       ├── image
│       ├── mount
│       ├── network
│       ├── registry
│       ├── strslice
│       ├── swarm
│       │   └── runtime
│       ├── time
│       ├── versions
│       └── volume
├── builder
│   └── remotecontext
│       └── urlutil
├── client
├── errdefs
├── pkg
│   ├── archive
│   ├── homedir
│   ├── idtools
│   ├── ioutils
│   ├── jsonmessage
│   ├── longpath
│   ├── namesgenerator
│   ├── pools
│   ├── stdcopy
│   └── system
└── registry

From the above, I wonder if we need to create test-PRs that;

  1. update buildkit to use this PR (and perhaps even update moby to use that)
  2. update buildx to use this PR (and 1?)
  3. update compose to use this PR (and 2?)

@thaJeztah thaJeztah marked this pull request as draft April 1, 2023 15:41
@thaJeztah
Copy link
Member

I moved this back to draft to prevent accidental merges, but let's continue the conversation, and draft a plan (I'm happy to see this part works at least; let's continue with the other parts to know / understand the full impact, and to see how we can move forward)

@rfay
Copy link

rfay commented Feb 21, 2024

Super important, would love to see this land. This is critical to the community, so that people can do "normal" building and debugging.

@rfay
Copy link

rfay commented Feb 21, 2024

@crazy-max - thanks for this great work. Would you mind rebasing it against current? It's super important to all of us.

@rfay
Copy link

rfay commented Mar 1, 2024

Just having a rebased PR would allow those of us who use normal tooling to debug through the cli and study its behavior. thanks!

@chenrui333
Copy link

at this point, can one of the maintainers take over it to the new pr or forced update this pr? Thanks!

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.

5 participants