Improve makefile situation

@dnephin

A couple of issues with Makefiles in this repo:

The first issue is that they are full of @ output suppression. A lot of the targets execute for a long time and I can't see what is happening or if anything is stuck. Also docker build uses -q for some reason.

The other problem is the duplication of content in multiple Makefiles and aggressive use of mounts. Bind mounts break all cases when building against a remote API and they are almost never needed. By not using mounts you have similar performance and better caching. Similarily I see no reason to define these local duplicate targets that can easily go out of sync and have no guarantees of correctness as they rely on the state of host system. By looking at issues in this repo there are already multiple reports of these problems showing up.

My proposal is:

Replace Makefile with docker.Makefile
Add a build target that copies artifacts back to the host to keep the current behavior. GOOS can be sent with a build arg.
Same for vendor and go generate target
Build step should run as part of a Dockerfile not in a separate container. Target stages can be used to build without tests or get devenv.
Dev-env target can keep using the mounts(for now)
Separate Dockerfile for CI should not be needed. At least after ci supports multi-stage builds if we want to use some better syntax for the local version.

@dnephin

I think you're looking at an older version? Some of these issues are already fixed.

the docker build -q is fixed
the Dockerfile.ci is gone, which means just about all the duplication is gone

I agree with removing the @ from Makefile targets, the output is valuable.

Bind mounts break all cases when building against a remote API

bind mounts are necesasry. See #87. There's no reason to support building against a remote API.

and better caching

What is the better caching? I don't know of any operation that can be cached that isn't already.

Similarly I see no reason to define these local duplicate targets that can easily go out of sync and have no guarantees of correctness as they rely on the state of host system.

Do you mean the targets in Makefile? They are not local targets, they are the "in-container" targets. We can definitely remove all the duplication with my build.sh proposed solution below.

Replace Makefile with docker.Makefile

I think this is the wrong idea. We started some discussion about this here: #78 (comment)

Makefiles are a bad choice for this use case. We should replace docker.Makefile with a ./build.sh until we have a better tool.

Add a build target that copies artifacts back to the host to keep the current behavior. GOOS can be sent with a build arg.

Not sure what you mean by this. What is the "current" behaviour? Where do we need to set GOOS ?

Build step should run as part of a Dockerfile not in a separate container.

I do not agree with this. There's no reason to build in as a build stage. It makes it much slower to develop. Many tasks do not require the binary. Unless you're building an image for distribution with a binary in it, there is no reason to compile in the Dockerfile. What's the advantage to this?

When we have a DAG builder, sure we can do this, but until then compiling in the Dockerfile is significant worse.

Target stages can be used to build without tests or get devenv.

Our CI doesn't support targets yet. I would let to use them, but let's get CI working first.

Some of these issues are already fixed.

👍

bind mounts are necesasry

You can keep it for watch target. As I said, dev-env target is fine as well.

What is the better caching?

If you are running a repeated operation without the content of specific sources changing you get a quick response. Nothing running docker run or -v can do that.

I think this is the wrong idea. Makefiles are a bad choice for this use case.

Nobody knows what is inside your build.sh. Everyone knows how to use make or docker build. We should leverage docker build caching instead of Makefile when there is no file target.

Not sure what you mean by this. What is the "current" behavior? Where do we need to set GOOS ?

The current behavior is that a binary appears in the host after make binary(nobody knows if it will actually succeed though or what go version it uses). That behavior can be kept with doing docker build. You just need to docker cp the binary to local dir(until there is a builtin way for docker build to do that). Even more important for vendor as vndr makes incompatible updates and doesn't work from every GOPATH.

There's no reason to build in as a build stage.

See cache above, there is no performance hit when files do not change. What is the case for running tests(or even lint) for a code that doesn't build? For your dev-env/watch case you are only running this once and after finishing and running the tests, it is a good idea to make sure that your clean version builds as well. By not even putting the source in the Dockerfile you are effectively stepping out of docker build into your host dependant environment.

My dev experience after fresh clone:

 # make binary
make: *** [binary] Error 1
 # make -f docker.Makefile binary
...
Successfully built 20606fbe7545
Successfully tagged docker-cli-dev:latest
WARNING: this will drop a Linux executable on your host (not a macOS or Windows one)
make: *** No rule to make target 'binary'.  Stop.
make: *** [binary] Error 2

I see a binary target on master, and that command works for me from a fresh clone. I'm not sure what's up with that. You sure you cloned docker/cli ?

I understand that with many of the proposal we have open for the builder we should be able to implement some of these workflows as you describe. Unfortunately right now making these changes would make the development experience significant worse.

There are plenty of issues that make docker build a non-optimal workflow:

caching is not fine grained enough. I can't ADD/COPY files based on some regex or file list. If I edit some non-go file, my cache breaks for no reason.
without a DAG every operation is slow. there is not proper linear sequence of events that is correct. It must be a graph.
these are common operations that developers have to do dozens of times a day. A command like docker build --target something . is the absolute best case, and even that is way too long. Many cases will require tags, build args, a dockerfile path, etc. docker cp makes this even worse. I shouldn't have to remember all of this detail to perform some task. It needs to be <tool> <taskname>
right now uploading the build context is a huge productivity tax. Until we have the incremental context sending, mounts are necessary

re: caching, do you have examples of where that kind of cache is actually useful? I can't think of a case where I would run something like go test without changing some file. And as I mention in point 1 above, 90% of the time the cache is going to break on unrelated changes anyway.

I will look at fixing the @ and the default makefile tomorrow. Part of my suggestion was also to add a warning to the file so that anyone that tries to run it without a proper environment is directed to the correct place.

Renaming it will make it worse for the case where you run a long running dev-env container, which is one of the most common workflows for docker/docker.

By not even putting the source in the Dockerfile you are effectively stepping out of docker build into your host dependant environment.

Not sure what you mean by this. Everything runs in a container, nothing runs on the host.

It is failing because there is no source in the image, therefore no makefile nor target. I know that because I looked at what the target does but no first time contributor will do that. That's what I meant by host dependency - you are relying on files/setup of your client instead of docker api. docker build is designed exactly to fix this common problem. I have not looked why the local one fails as well, I do have go installed.

caching is not fine grained enough.

How is this worse than having no caching with docker run? You can get fine-grained cache with an extra stage that does filter but I don't think it is needed for a simple setup like this.

without a DAG every operation is slow.

It would be better with DAG but I don't get why you say it is slower than a one-off process. The cache is not linear even atm.

docker cp makes this even worse. I shouldn't have to remember all of this detail

docker cp would be inside the make target, written once. 99% of users would never know that it is there. They only know that it always works(unlike now).

right now uploading the build context is a huge productivity tax. Until we have the incremental context sending mounts are necessary

shameless plug: incremental context sending is ready for review.

with 17.05:

# time docker build -f ./dockerfiles/Dockerfile.dev .
docker build -f ./dockerfiles/Dockerfile.dev .  0.16s user 0.12s system 52% cpu 0.529 total

Again, I have nothing against the current dev-env or watcher targets. I agree that we don't have a very good solution yet. It's an advanced case and it's fine if it requires more complicated setup. What is not acceptable is that there is no foolproof way to run the critical actions for contributing: build, test, lint, vendor. And it is sad because we make a product to solve that problem.

I also don't think that anyone should never run anything in the host. If you know what you are doing, run go build in your host. But we don't need to promote that in the makefile.

do you have examples of where that kind of cache is actually useful?

Even with COPY . .: switching branches, restarting dev-env.

@tonistiigi

See cache above, there is no performance hit when files do not change. What is the case for running tests(or even lint) for a code that doesn't build?

@tonistiigi mainly test-driven developpement. I tend to write my test, pass them (at package level) and then looking at the whole compile issue and lint stuff. But yeah can be on a specific target like watch.

Overall I tend to agree on @tonistiigi on making the most out of docker build (mainly because it's easier to support dev/contribution from windows if docker build makes it possible to run a lot — even if slower for some actions). I see the value to have a docker build that builds the binary and run the tests "out-of-box" and have Makefile targets to support more advanced/agile setup.

How is this worse than having no caching with docker run?

It's worse because docker run can use make or some other build tool to properly cache the operation. If you do COPY . . you're breaking the cache every time. So by not adding source to the image you get optimal builds. The image is almost always fully cached, and the appropriate tools are able to do fine grained caching.

It would be better with DAG but I don't get why you say it is slower than a one-off process.

There's no order of build steps that you can put into stages that will give you an optimal build. If I just want to run unit tests and use --target unit-test, I have to repeat all the previous stages, which will include steps I don't need. If I re-arrange the stages I have the same problem when I run a different task. If I'm trying to perform one action I don't need to perform every other action every time.

docker cp would be inside the make target, written once. 99% of users would never know that it is there.

Why a makefile if every target is .PHONY ? It's not the right tool. Running things from a makefile on the host is also not portable (which is the same problem with a bash script, but at least bash is more likely to be installed). dobi on the other hand would work for windows users.

It's an advanced case and it's fine if it requires more complicated setup.

I don't think it's that advanced. I think it should be the recommended workflow since it's optimal.

What is not acceptable is that there is no foolproof way to run the critical actions for contributing: build, test, lint, vendor.

I agree. We should address this issue directly. From what I understand the issues you are facing are because you're using a remote engine, is that correct?

And it is sad because we make a product to solve that problem.

Yes, I agree that is unfortunate. But we've never invested any time in making a good tool for developer workflows, which is why I wrote dobi. At least now we are making progress to fix these problems.

If you know what you are doing, run go build in your host. But we don't need to promote that in the makefile.

Yup, I completely agree. That's why the separation of the two makefiles is so important right now. Makefile can be used on the host (if you do the work to install all the necessary things), or from inside a container (which we know always provides the necessary things).

If you do COPY . . you're breaking the cache every time. So by not adding source to the image you get optimal builds. The image is almost always fully cached, and the appropriate tools are able to do fine grained caching.

How is COPY . breaking the cache for anything before that. So any part that is currently "fully cached"?

It would be better with DAG

Until we have DAG we could have the most basic workflow build->lint->test in that order. Test and lint are just invoked by a single command so you can have a special makefile target that just invokes the command on top of the base stage. As you said "the recommended" workflow is to run a long dev-env anyway so there shouldn't be a problem.

Why a makefile if every target is .PHONY ? It's not the right tool.

Using docker build stateful caching is the solution for the .PHONY targets that can't use file timestamps. Getting docker build full cache hit takes only 500ms in 17.05 and likely this number will soon go down 10x. There are some examples of targets that are not phony and binary/cross definitely would not be phony if make target and source files would be defined. Makefile is used here because it is very popular and covers the use case for launching dev tasks.

If you know what you are doing, run go build in your host. But we don't need to promote that in the makefile.

Yup, I completely agree. That's why the separation of the two makefiles is so important right now.

I don't think you agree with the second part then. The current default that we recommend is running things in host. This is what default Makefile in root does.

How is COPY . breaking the cache for anything before that. So any part that is currently "fully cached"?

Yes, anything before a COPY/ADD is identical for both strategies, so I'm ignoring that part of the build.

The relevant part is what comes after COPY/ADD (or in this case, is done during run with a mount). My comment was just referring to the cache after the COPY/ADD.

Until we have DAG we could have the most basic workflow build->lint->test in that order

Why would I build before lint or test? I think that's backwards, but that's also kind of my point. A linear sequence is never going to be correct. Also lint uses a different image because it has different dependencies.

The current default that we recommend is running things in host. This is what default Makefile in root does.

Where do we recommend that? I fixed the README so that it doesn't mention the host anymore.

I'm looking to add a warning to the Makefile so that it's clear. Renaming would also work, but I think it makes it worse for the in-container development.

Using docker build stateful caching is the solution for the .PHONY targets

I was referring to the docker.Makefile which is completely .PHONY. This is the makefile that runs docker build, so I don't see how we can use docker build to fix it.

My comment was just referring to the cache after the COPY/ADD.

After copy there will only be the command that is currently in docker run

Why would I build before lint or test?

Good defaults. Passing lint or test is useless if the project doesn't build.

Where do we recommend that?

I clone the repo. I see there is a Makefile (and no Dockerfile), I type make <tab>, see a binary target and press enter(and get an error). Host one is the default, dockerfile one is the override.

Merge pull request docker#99 from GordonTheTurtle/sync-with-upstream-…

Closing as out of date/not really relevant, though the build system still needs more work.

Merge pull request docker#99 from andrewhsu/redact

Navigation Menu

Repository navigation

Improve makefile situation #99

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Improve makefile situation #99

Description

Activity

dnephin commented on May 17, 2017

tonistiigi commented on May 17, 2017

dnephin commented on May 17, 2017

dnephin commented on May 17, 2017

tonistiigi commented on May 17, 2017

vdemeester commented on May 17, 2017

dnephin commented on May 17, 2017

tonistiigi commented on May 17, 2017

dnephin commented on May 17, 2017

dnephin commented on May 17, 2017

tonistiigi commented on May 17, 2017

neersighted commented on May 12, 2023

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions