makefiles/docker: fix mounting localtime on OSX #10568

smlng · 2018-12-06T19:38:08Z

Contribution description

Mounting /etc/localtime directly does not work on macOS (anymore).
However, by resolving the symlink to its real path docker can handle
the mount on macOS again - should also work with Linux.

Testing procedure

Try to run BUILD_IN_DOCKER=1 make -C tests/minimal on macOS, which will fail with one or the other error regarding either that /etc is not shared and hence not accessible by docker, or if you do add it to the list of shared folders, it will fail with mounting /etc/localtime into docker. With this PR it works again.

Issues/PRs references

Fixes #10287.

aabadie · 2018-12-06T19:59:40Z

Is this an alternative to #10564 ? (or a duplicate ?)

smlng · 2018-12-06T21:19:12Z

its a duplicate in a way ... at least I was slower in opening the PR here. I worked on it today and found a solution, made a branch in RIOT and posted the solution in the respective issue at docker/for-mac but haven't opened the PR right away.

Seems like @jthacker was quicker with the PR ... though I find my solution with the variable cleaner.

Anyway, we can close this one I guess.

jcarrano

I like this approach better than #10564 as having a separate variable makes it easier to see and document it is a workaround for a buggy docker.

makefiles/docker.inc.mk

jcarrano · 2018-12-07T12:28:00Z

makefiles/docker.inc.mk

@@ -94,6 +94,9 @@ _is_git_worktree = $(shell grep '^gitdir: ' $(RIOTBASE)/.git 2>/dev/null)
 GIT_WORKTREE_COMMONDIR = $(shell git rev-parse --git-common-dir)
 DOCKER_VOLUMES_AND_ENV += $(if $(_is_git_worktree),-v $(GIT_WORKTREE_COMMONDIR):$(GIT_WORKTREE_COMMONDIR))

+# Resolve symlink of /etc/localtime to its real path
+ETC_LOCALTIME := $(shell realpath /etc/localtime)


There is no need for immediate assignment here

Suggested change

ETC_LOCALTIME := $(shell realpath /etc/localtime)

ETC_LOCALTIME = $(shell realpath /etc/localtime)

I think here simply the $(realpath function can be used as it should resolve symlinks.

cladmi · 2018-12-07T12:33:10Z

Question here, there were many PRs before, to not use realpath because OSX does not have it. Why using it now ?

smlng · 2018-12-07T12:39:59Z

@cladmi you're right, but the other problem is though readlink exists on Linux and macOS is does not behave the same. That is Linux' realpath == macOS's readlink != Linux' readlink.

So we need to distinguish between OS here, I'd say.

smlng · 2018-12-07T12:40:59Z

To really solve this once and for all RIOT needs a proper system init, that is: check the OS and lookup all the tools and stuff and set ENV accordingly.

jcarrano · 2018-12-07T12:49:35Z

Does make's inbuilt realpath work for this?

Also, the question here is, did you have to install realpath (from GNU coreutils)? Can we count on that being present on a every mac? After a previous PR (#10195) I was under the impression that it is not standard in Mac.

makefiles/docker.inc.mk

jcarrano · 2018-12-07T12:55:50Z

It does not break anything on linux, squash and let's move forward.

aabadie · 2018-12-07T12:57:14Z

Funny to see that except the variable, after reviews this PR is now proposing the same fix as in #10564.

smlng · 2018-12-07T12:59:31Z

@aabadie it was mainly about the variable from the beginning, but still you're ~~still~~ right. I'm not eager to take away the contribution of the other PR ... how to proceed?

Mounting `/etc/localtime` directly does not work on macOS (anymore). However, by resolving the symlink to its real path docker can handle the mount.

smlng · 2018-12-07T13:00:53Z

[the variable] and documentation

jcarrano · 2018-12-07T13:53:44Z

Funny to see that except the variable, after reviews this PR is now proposing the same fix as in #10564.

True, even the PR/commit title was nicer in the other one. Anyways, this is not a competition to see who gets his fix first, it is about getting fixes done, I'm sure @jthacker will be happy that the issue is solved, whoever authors the patch is secondary.

jthacker · 2018-12-07T14:40:21Z

@jcarrano @smlng I am fine with this, just happy to see the fix make it into master.

jcarrano

I tested it does not break Linux. The only possible drawback of this is that now the (very approximate) location of the user is printed on the command line and therefore in build logs.

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

smlng requested review from cladmi and jcarrano December 6, 2018 19:38

jcarrano suggested changes Dec 7, 2018

View reviewed changes

smlng force-pushed the pr/make/docker branch from b98acd8 to a18788e Compare December 7, 2018 12:52

jcarrano reviewed Dec 7, 2018

View reviewed changes

makefiles/docker.inc.mk Outdated Show resolved Hide resolved

make/docker: adapt mount of localtime

cae865e

Mounting `/etc/localtime` directly does not work on macOS (anymore). However, by resolving the symlink to its real path docker can handle the mount.

smlng force-pushed the pr/make/docker branch from cefbc10 to cae865e Compare December 7, 2018 13:00

jcarrano changed the title ~~make/docker: adapt mount of localtime~~ makefiles/docker: fix mounting localtime on OSX Dec 7, 2018

jcarrano mentioned this pull request Dec 7, 2018

makefiles/docker: fix docker build on osx #10564

Closed

jcarrano approved these changes Dec 7, 2018

View reviewed changes

jcarrano merged commit d505b01 into RIOT-OS:master Dec 7, 2018

smlng deleted the pr/make/docker branch December 10, 2018 08:08

aabadie added this to the Release 2019.01 milestone Dec 19, 2018

cladmi mentioned this pull request Jul 9, 2019

murdock: allow multiple files to be sent along with a test job #11697

Merged

LukeMondy pushed a commit to seL4/seL4-CAmkES-L4v-dockerfiles that referenced this pull request Sep 14, 2020

Fix localtime issue on OSX.

80f6045

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

LukeMondy pushed a commit to seL4/seL4-CAmkES-L4v-dockerfiles that referenced this pull request Sep 14, 2020

Fix localtime issue on OSX.

fbcfe92

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

LukeMondy pushed a commit to seL4/seL4-CAmkES-L4v-dockerfiles that referenced this pull request Dec 20, 2020

Fix localtime issue on OSX.

a8f5f9c

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

LukeMondy pushed a commit to seL4/seL4-CAmkES-L4v-dockerfiles that referenced this pull request Dec 20, 2020

Fix localtime issue on OSX

cccd958

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

LukeMondy pushed a commit to seL4/seL4-CAmkES-L4v-dockerfiles that referenced this pull request Dec 20, 2020

Fix localtime issue on OSX

a9bb8af

Copied solution from: RIOT-OS/RIOT#10568 Signed-off-by: Luke Mondy <luke.mondy@data61.csiro.au>

	ETC_LOCALTIME := $(shell realpath /etc/localtime)
	ETC_LOCALTIME = $(shell realpath /etc/localtime)

makefiles/docker: fix mounting localtime on OSX #10568

makefiles/docker: fix mounting localtime on OSX #10568

Uh oh!

Conversation

smlng commented Dec 6, 2018 • edited by jcarrano Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

aabadie commented Dec 6, 2018

Uh oh!

smlng commented Dec 6, 2018

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcarrano Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi Dec 7, 2018

Choose a reason for hiding this comment

Uh oh!

cladmi commented Dec 7, 2018

Uh oh!

smlng commented Dec 7, 2018

Uh oh!

smlng commented Dec 7, 2018

Uh oh!

jcarrano commented Dec 7, 2018

Uh oh!

Uh oh!

jcarrano commented Dec 7, 2018

Uh oh!

aabadie commented Dec 7, 2018

Uh oh!

smlng commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

smlng commented Dec 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcarrano commented Dec 7, 2018

Uh oh!

jthacker commented Dec 7, 2018

Uh oh!

jcarrano left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

smlng commented Dec 6, 2018 •

edited by jcarrano

Loading

smlng commented Dec 7, 2018 •

edited

Loading

smlng commented Dec 7, 2018 •

edited

Loading