Skip to content

makefiles/docker: fix mounting localtime on OSX#10568

Merged
jcarrano merged 1 commit intoRIOT-OS:masterfrom
smlng:pr/make/docker
Dec 7, 2018
Merged

makefiles/docker: fix mounting localtime on OSX#10568
jcarrano merged 1 commit intoRIOT-OS:masterfrom
smlng:pr/make/docker

Conversation

@smlng
Copy link
Copy Markdown
Member

@smlng smlng commented Dec 6, 2018 •
edited by jcarrano
Loading

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.

@smlng smlng added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: build system Area: Build system OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Dec 6, 2018
@smlng smlng requested review from cladmi and jcarrano December 6, 2018 19:38
@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 6, 2018

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

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Dec 6, 2018

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
jcarrano suggested changes Dec 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated
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)
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano Dec 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for immediate assignment here

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

Copy link
Copy Markdown
Contributor

@cladmi cladmi Dec 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cladmi
Copy link
Copy Markdown
Contributor

cladmi commented Dec 7, 2018

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

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Dec 7, 2018

@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
Copy link
Copy Markdown
Member Author

smlng commented Dec 7, 2018

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
Copy link
Copy Markdown
Contributor

jcarrano commented Dec 7, 2018

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.

jcarrano
jcarrano reviewed Dec 7, 2018
View reviewed changes
@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented Dec 7, 2018

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

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Dec 7, 2018

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

@smlng
Copy link
Copy Markdown
Member Author

smlng commented Dec 7, 2018 •
edited
Loading

@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?

@smlng
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
Copy link
Copy Markdown
Member Author

smlng commented Dec 7, 2018 •
edited
Loading

[the variable] and documentation

@jcarrano jcarrano changed the title make/docker: adapt mount of localtime makefiles/docker: fix mounting localtime on OSX Dec 7, 2018
@jcarrano
Copy link
Copy Markdown
Contributor

jcarrano commented Dec 7, 2018

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.

@jcarrano jcarrano mentioned this pull request Dec 7, 2018
Closed
@jthacker
Copy link
Copy Markdown
Contributor

jthacker commented Dec 7, 2018

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

jcarrano
jcarrano approved these changes Dec 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@jcarrano jcarrano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@jcarrano jcarrano merged commit d505b01 into RIOT-OS:master Dec 7, 2018
@smlng smlng deleted the pr/make/docker branch December 10, 2018 08:08
@aabadie aabadie added this to the Release 2019.01 milestone Dec 19, 2018
@cladmi cladmi mentioned this pull request Jul 9, 2019
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cladmi cladmi Awaiting requested review from cladmi

1 more reviewer

@jcarrano jcarrano jcarrano approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Area: build system Area: Build system CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR OS: Mac OS X Host OS: This PR/issue concerns usage of RIOT with Mac OS X as a host system Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Milestone

Release 2019.01

Development

Successfully merging this pull request may close these issues.

5 participants

@smlng @aabadie @cladmi @jcarrano @jthacker