Replace makefile with a bash script

[dnephin]

Related to #99

Replace a 70 line .PHONY Makefile with 40 lines of bash which actually does a lot more than the original Makefile. This bash script will allow the addition of new targets to the Makefile without having to touch ./tasks.

shell prompts are provided to all images without any additional configuration.

I don't necessarily think this is the best solution, but I think it's an improvement over the Makefile.

[dnephin]

Another improvement that could be made is to handle the case where DOCKER_HOST is set to a TCP socket. We could build the images with an added COPY . . and not use mounts. This would allow both workflows.

[mlaventure]

Definitely an improvement.

A few suggestions:

• the README.md need to be updated with instructions on how to use this script
• include a help/usage message for people not reading the README

[dnephin]

Fixed the README and added a usage.

[mlaventure]

couple of nits, but LGTM.

[mlaventure]

use

make -qp | awk -F':' '/^[a-zA-Z0-9][^$#\/\t=]*:([^=]|$)/ {split($1,A,/ /);for(i in A)print A[i]}'  | sort -u

To display the Makefile targets? (Stolen from SO).

[dnephin]

I was looking at that too. My concern is that this runs on the host, so now instead of just requiring bash on the host we'd also be requiring make, awk, and sort (with versions that support those flags). I guess in most cases anyone with bash would have awk and sort, but possible not make.

[mlaventure]

can have it test for make and awk presence and use your message if it is not present

[mlaventure]

I'm confusing by what this means 😅. Can it be simplified?

Something like:

The ./tasks script allows one to build and develop the cli via Docker images. For local development, the Makefile can be used directly with the same commands described below for ./tasks.

This can probably be made better too though. cc @mstanleyjones ? :)

[tonistiigi]

Another improvement that could be made is to handle the case where DOCKER_HOST is set to a TCP socket. We could build the images with an added COPY . . and not use mounts. This would allow both workflows.

Yes, please. Why can only circle https://github.com/docker/cli/blob/master/circle.yml#L19 build things correctly and not me? Btw, that line seems like serious anti-pattern, we are never testing the pristine source code in CI. Actually there is no place in local development either where I could verify that build/test ran on an unmodified source. If you use mounts locally then you never know what was used as source as these are mutable in any stage.

I'm bit confused why we need to reinvent the wheel here. Makefile is very popular, much more readable than bash, not less windows friendly than bash. Has nice features like completion, sane logging and query features. Atm, to get any reasonable results I have to use it with make MOUNTS= that I couldn't do with this script. Of course you can add support for that but it will be just another reimplementation of a builtin feature. Another example is running make cross dynbinary that would only call docker build once while this script can't make this optimization.

We already discussed it in #99 but one the problems I have is that we use wrong defaults. The default operations known for every developer are the ones that don't work. You have to read the docs to see that although there is a Makefile you should not run it but use some script instead. Ideally, I can run just make or make all and it will run all the operations needed to verify my changes are ready for contribution, in a way that works consistently with every setup. If I need to use a watcher, filter or whatever advanced workflow I can read the docs and invoke scripts if needed.

[dnephin]

Ok, I can add the DOCKER_HOST check for TCP. That should allow us to run this script in CI, which is great.

we are never testing the pristine source code in CI.

I don't understand this statement. How are we not? We're not making any modifications to the source files. The Dockerfiles are development configs, not the object under test. If the issue is the echo ... >> Dockerfile that will be fixed by the above change.

Actually there is no place in local development either where I could verify that build/test ran on an unmodified source.

What do you mean by unmodified? The entire purpose of development is to modify the source, so I want my tests to run on what is in my current directory, nothing else.

I'm bit confused why we need to reinvent the wheel here. Makefile is very popular, much more readable than bash, not less windows friendly than bash. Has nice features like completion, sane logging and query features.

I think we are greatly overestimating the popularity of makefiles. They are popular for a lot of c/c++ projects where they are a good tool for the job. They are not a good tool for running docker commands.
I don't think bash is the best solution, but it's an improvement over a Makefile that is full of .PHONY targets.

The default operations known for every developer are the ones that don't work.

If I understand this argument it's that you expect make to be the entry point of every project. I think we can fix that one case by having make default to warning that you should use ./tasks.

We aren't actually using any of the features of make. It is basically a heavily duplicated bash script. It seems very silly to me that we would make this a suggested workflow.

[cpuguy83]

If we aren't going to use make as an entrypoint then we should remove the Makefile.

[dnephin]

The Makefile is not useful as an entrypoint tothe project (all targets are .PHONY), but it is useful as a tool inside the container, for file-based operations. I think we should use it where it provides value.

Instead of removing it I'd like to add a warning to the all target to direct users to the ./tasks script. We could rename it, but unfortunately make doesn't seem to support setting the default filename from an environment variable, so this would make it less convenient to run commands in the interactive development container.

We could also rename it and alias make -> make -f whatever, but I'm not sure that's an improvement.

WDYT?

[tonistiigi]

The Dockerfiles are development configs, not the object under test.

Dockerfile's are part of the source same way as any other committed file. Yes, my comment was about that specific line.

The entire purpose of development is to modify the source, so I want my tests to run on what is in my current directory, nothing else.

The purpose is also to verify that specific commit passes all the jobs. If some random code-state that you can't reproduce or verify passed a test it doesn't provide much value.

Makefile that is full of .PHONY targets

What's so bad about .PHONY, compared to the script?

[dnephin]

What's so bad about .PHONY, compared to the script?

I think the question is really the opposite. Why use a tool (make) that does nothing more than a bash script with lots of duplication? If every target is .PHONY what feature of make are we really using?

From this thread and the related issue I think it's clear that we have different workflows. The bash script allows us to support those different workflows without duplicating every target.

The bash script allows us to optionally enable -ti when it's run in a terminal. To add new tasks without adding duplicating the docker build+run lines.

It also lets us experiment to figure out what we need from a better dev tool that we can use to replace ./tasks. A Makefile is very rigid and doesn't allow us to create the workflow we need.

[dnephin]

Ok, I've made the following changes:

• add a list of targets to usage
• fix the wording of the README
• use ./tasks in CI, which removes a bunch of config from circle.yml and allows us to verify that ./tasks works correctly
• add a check to ./tasks for $DOCKER_HOST. If it's set then bind mounts are not used. Bind mounts can also be disabled with $NO_BINDMOUNT.

[mlaventure]

while you're at it, this need to export VERSION, GITCOMMIT. BUILDTIME and LDFLAGS from the env (to match what's in scripts/build/.variables)

[dnephin]

added, thanks!

[dnephin]

I think the right long term solution for this will be to take what we end up with in bash, write it in GO , and extract anything that is project specific into a config file. That should produce a good prototype for a project tasks tool.

[tonistiigi]

I think the question is really the opposite. Why use a tool (make) that does nothing more than a bash script with lots of duplication? If every target is .PHONY what feature of make are we really using?

.PHONY is a useful and very popular feature of Makefiles to use make features and definition even if the target doesn't map to an actual file. The fact that .PHONY targets are so common just shows how many benefits make has even when there are no timestamp comparisons.

This is definitely an improvement, I can build/test again without modifying the source. 👍 for that.

[dnephin]

rebased

[codecov-io]

Codecov Report

Merging #107 into master will increase coverage by 0.08%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #107      +/-   ##
==========================================
+ Coverage   48.85%   48.93%   +0.08%     
==========================================
  Files         186      177       -9     
  Lines       12413    11846     -567     
==========================================
- Hits         6064     5797     -267     
+ Misses       5975     5687     -288     
+ Partials      374      362      -12

[dnephin]

I've got 2 LGTM on this, so moving to code review

[dnephin]

Looks like circleCI isn't running on some PRs, looking into it

[dnephin]

Replaced by #343. This PR seemed to be broken. github wasn't triggering any hooks.