Page MenuHomeFreeBSD

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

Authored by lifanov on Sep 7 2017, 6:14 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 22, 10:14 AM
Unknown Object (File)
Fri, Apr 19, 12:48 PM
Unknown Object (File)
Mar 17 2024, 7:37 PM
Unknown Object (File)
Feb 23 2024, 4:54 PM
Unknown Object (File)
Feb 13 2024, 3:14 PM
Unknown Object (File)
Feb 2 2024, 1:49 AM
Unknown Object (File)
Jan 31 2024, 5:38 AM
Unknown Object (File)
Jan 26 2024, 8:44 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 Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 11421
Build 11780: arc lint + arc unit

Event Timeline

list myself as maintainer per dteske

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

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

I shuffled things around, so bugs are all mine.

per dteske, it's fine to just use docker_dir

sysutils/docker-freebsd/files/docker.in
31–36 ↗(On Diff #32782)

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

38 ↗(On Diff #32782)

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

72–84 ↗(On Diff #32782)

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

91 ↗(On Diff #32782)

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

107–108 ↗(On Diff #32782)

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)

address some of dteske's feedback

lifanov added inline comments.
sysutils/docker-freebsd/files/docker.in
107–108 ↗(On Diff #32782)

It still doesn't fit in 79 cols :(

address dteske's feedback: localize "module" var

sysutils/docker-freebsd/files/docker.in
31–37 ↗(On Diff #32785)

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=

39 ↗(On Diff #32785)

Preferred for minimal diffs:

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

134 ↗(On Diff #32785)

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

134–138 ↗(On Diff #32785)

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

107–108 ↗(On Diff #32782)

Thanks for checking ;D

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 added inline comments.
sysutils/docker-freebsd/files/docker.in
39 ↗(On Diff #32785)

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

134 ↗(On Diff #32785)

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

address dteske's feedback

sysutils/docker-freebsd/Makefile
5–7

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

20

Use USE_GITHUB=yes.

21

default value, remove.

25–36

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

sysutils/docker-freebsd/Makefile
5–7

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

sysutils/docker-freebsd/Makefile
5–7

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.

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

sysutils/docker-freebsd/files/docker.in
39 ↗(On Diff #32785)

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.

134 ↗(On Diff #32785)

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).

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

sysutils/docker-freebsd/Makefile
18–21

Remove, default value.

sysutils/docker-freebsd/files/docker.in
107–108 ↗(On Diff #32782)

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.

sysutils/docker-freebsd/files/docker.in
107–108 ↗(On Diff #32782)

Religion called, they want their hate back.

sysutils/docker-freebsd/Makefile
18–21

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.

sysutils/docker-freebsd/files/docker.in
138–139 ↗(On Diff #32810)

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

146 ↗(On Diff #32810)

$docker_pidfile instead of hard-coded value repeated throughout

151 ↗(On Diff #32810)

$docker_pidfile instead of hard-coded value repeated throughout

153 ↗(On Diff #32810)

$docker_pidfile instead of hard-coded value repeated throughout

address dteske's feedback: un-hardcode pid file

sysutils/docker-freebsd/Makefile
18–21

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

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.