Page MenuHomeFreeBSD

Update sysutils/docker-freebsd to somewhere circa v17.05.0
AcceptedPublic

Authored by lifanov on Sep 7 2017, 6:14 PM.

Details

Reviewers
dteske
Summary
Update to latest from https://github.com/freebsd-docker project.

Submitted by:	dteske
Test Plan
portlint - OK
build - OK
poudriere testport - OK
run - not tested yet

Diff Detail

Repository
rP FreeBSD ports repository
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11492
Build 11847: arc lint + arc unit

Event Timeline

lifanov created this revision.Sep 7 2017, 6:14 PM
lifanov updated this revision to Diff 32777.Sep 7 2017, 6:17 PM

list myself as maintainer per dteske

lifanov updated this revision to Diff 32779.Sep 7 2017, 6:24 PM

s/post-install/do-install/ - it makes more sense this way

lifanov updated this revision to Diff 32781.Sep 7 2017, 6:58 PM

include dteske's changes to the rc script and wrap them im prestart

I shuffled things around, so bugs are all mine.

lifanov updated this revision to Diff 32782.Sep 7 2017, 6:59 PM

per dteske, it's fine to just use docker_dir

lifanov edited the test plan for this revision. (Show Details)Sep 7 2017, 7:24 PM
dteske added inline comments.Sep 7 2017, 7:28 PM
sysutils/docker-freebsd/files/docker.in
32–39

We should put "local" (without quotes) before each of these (non-blank) lines

40–48

We'll want a "local module" (w/o quotes) at the top of the function (first line after curly) to localize the variable used here in the for-loop

79–91

Let's reduce the indentation of these lines by a single tab

98

Let's remove this line -- it's not necessary unless using the dpv/cat pipe-through to redirect method

114–115

Since renaming docker_zfs_dir to docker_dir (reduction of 4 letters), is that enough to fit this all one one line that is 79 characters-or-less? (if no, retain line continuation)

lifanov updated this revision to Diff 32784.Sep 7 2017, 7:43 PM

address some of dteske's feedback

lifanov marked 4 inline comments as done.Sep 7 2017, 7:44 PM
lifanov added inline comments.
sysutils/docker-freebsd/files/docker.in
114–115

It still doesn't fit in 79 cols :(

lifanov marked an inline comment as done.Sep 7 2017, 7:47 PM
lifanov updated this revision to Diff 32785.Sep 7 2017, 8:18 PM

address dteske's feedback: localize "module" var

dteske added inline comments.Sep 7 2017, 10:43 PM
sysutils/docker-freebsd/files/docker.in
32–39

Initialization to NULL is required for all but module and for purposes of minimal diffs, prefer making these separate statements

[tab]local module
[tab]local fs_type=
[tab]local make_zfs=
[tab]local zfs_mountpoint=
[tab]local zfs_name=
[tab]local zpool_name=
[tab]local zpool_size=

40–48

Preferred for minimal diffs:

[tab]for module in \
[tab][tab]zfs \
[tab][tab]pf \
[tab][tab]linux \
[tab][tab]linux64 \
[tab]; do

114–115

Thanks for checking ;D

140

There should be a space after > and 2> but more importantly, this is preferred:

[tab][tab]pgrep -F /var/run/docker.pid > /dev/null 2>&1

140–144

Preferred:

[tab][tab]if pgrep -F /var/run/docker.pid > /dev/null 2>&1; then
[tab][tab][tab]echo "Docker already running? /var/run/docker.pid"
[tab][tab][tab]exit 1
[tab][tab]fi

dteske retitled this revision from update docker-freebsd to somewhere circa v17.05.0 to Update sysutils/docker-freebsd to somewhere circa v17.05.0.Sep 7 2017, 10:50 PM
dteske edited the summary of this revision. (Show Details)
dteske edited the summary of this revision. (Show Details)
dteske edited the summary of this revision. (Show Details)
lifanov edited the summary of this revision. (Show Details)Sep 7 2017, 11:06 PM
lifanov marked 2 inline comments as done.Sep 8 2017, 2:01 PM
lifanov added inline comments.
sysutils/docker-freebsd/files/docker.in
40–48

You don't need a semicolon here if do is on the next line.

140

This isn't portable, but our sh supports it.

lifanov updated this revision to Diff 32798.Sep 8 2017, 2:03 PM

address dteske's feedback

lifanov marked 7 inline comments as done.Sep 8 2017, 2:05 PM
mat added inline comments.Sep 8 2017, 2:32 PM
sysutils/docker-freebsd/Makefile
5–6

Why add the g here that forces you to bump PORTEPOCH ?
This is a bad idea, remove the g.

22

Use USE_GITHUB=yes.

23

default value, remove.

25–35

Remove this from the GH_TUPLE and add GH_SUBDIR=src/github.com/docker/docker instead.

lifanov added inline comments.Sep 8 2017, 2:45 PM
sysutils/docker-freebsd/Makefile
5–6

This is going to become 17.05.0 before this patch lands, which will involve a PORTEPOCH bump.

mat added inline comments.Sep 8 2017, 2:52 PM
sysutils/docker-freebsd/Makefile
5–6

Well, when this becomes 17.05.0, then PORTEPOCH will be needed, but right now, with a version of 20170907, there is no need to do it.

lifanov updated this revision to Diff 32808.Sep 8 2017, 4:40 PM

address mat's feedback: clean up USE_GITHUB with default project

dteske added inline comments.Sep 8 2017, 5:11 PM
sysutils/docker-freebsd/files/docker.in
40–48

The reason why you should use:

[tab][tab]linux64 \
[tab]; do

Instead of:

[tab][tab]linux64
[tab]do

Is because list management (e.g., sorting with sort, moving items manually for ordered operation, etc.) is possible when all the elements have a trailing backslash. By making the last element not have a trailing backslash, a future diff that appends a new item to the end will not just be a diff with a single "+" line but instead a "-" and "+" pair where the backslash had to be added to the now-previous last-line in the list.

This also prevents mistakes.

140

What isn't portable? Space after the redirect operator? The duplication of fd1 to fd2 (to send stderr the same place as stdout)? Ordered file descriptor operations? All of those things are portable and no system I have touched in 20 years of using UNIX has ever balked at any of these. I would be shocked if you could find any variant of shell that balks at any of these (note: csh/tcsh/zsh don't count; things like ksh, dash, ash, do count -- it has to be a bourne shell variant, bash included since CentOS, Mac OS X, etc. use bash as /bin/sh).

lifanov updated this revision to Diff 32810.Sep 8 2017, 5:31 PM

address mat's feedback: un-bump epoch until we get a real version
address dteske's feedback: make for list sortable by including a semicolon

mat added inline comments.Sep 8 2017, 5:32 PM
sysutils/docker-freebsd/Makefile
20

Remove, default value.

sysutils/docker-freebsd/files/docker.in
114–115

The 80's called, they're telling me they had wider terminals back then and we have not had that limitations since before FreeBSD was born.

dteske added inline comments.Sep 8 2017, 5:36 PM
sysutils/docker-freebsd/files/docker.in
114–115

Religion called, they want their hate back.

dteske added inline comments.Sep 8 2017, 5:38 PM
sysutils/docker-freebsd/Makefile
20

The port is named docker-freebsd, so wouldn't that mean the default value would be backward? The GitHub account is freebsd-docker, not docker-freebsd.

dteske added inline comments.Sep 8 2017, 5:41 PM
sysutils/docker-freebsd/files/docker.in
139–140

Been thinking a lot that we should make the pidfile able to be set in rc.conf(5). E.g., docker_pidfile

147

$docker_pidfile instead of hard-coded value repeated throughout

152

$docker_pidfile instead of hard-coded value repeated throughout

154

$docker_pidfile instead of hard-coded value repeated throughout

lifanov updated this revision to Diff 32919.Sep 11 2017, 1:16 PM

address dteske's feedback: un-hardcode pid file

mat added inline comments.Sep 11 2017, 8:11 PM
sysutils/docker-freebsd/Makefile
20

Oh, yes, my brain rearranged the worlds in the same order, sorry about it.

dteske accepted this revision.Sep 11 2017, 10:15 PM

Great work! (can't think of anything else other than known pending issues e.g., PORT_EPOCH and version which is blocked by need to generate a tag; getting this far we can totally leverage this for runtime testing to get to a tag-point where we are happy with the runtime performance).

This revision is now accepted and ready to land.Sep 11 2017, 10:15 PM

Cool beans! Once the new version is functional enough to replace existing 1.7.0, I'll open a PR and attempt to land this.

emaste added a subscriber: emaste.Nov 16 2017, 4:09 AM