New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make utils_daemon and volumes.go cross-platform compileable (Windows). #11323

Merged
merged 1 commit into from Mar 16, 2015

Conversation

Projects
None yet
10 participants
@gewoonrik
Contributor

gewoonrik commented Mar 11, 2015

I have refactored the IsFileOwner function in utils_daemon to use system.GetStat and the cross-platform Stat type instead of (*syscall.Stat_t) (which does not exist on Windows). This change makes it possible to compile this file for Windows. However, GetStat will always return an error on Windows, because it is not implemented yet.

I have also added a test for the IsFileOwner function.

@gewoonrik gewoonrik changed the title from Make utils_daemon cross-platform compileable. to Make utils_daemon cross-platform compileable (Windows). Mar 11, 2015

@ahmetb

This comment has been minimized.

Show comment
Hide comment
@ahmetb

ahmetb Mar 11, 2015

Contributor
Contributor

ahmetb commented Mar 11, 2015

@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 12, 2015

Contributor

I have found another windows build incompatibility in copyOwnership in daemon/volumes.go and fixed it.
It now also uses system.GetStat and the system.Stat type.
The change is added to this PR, because it depends on the added GetStat function.

Contributor

gewoonrik commented Mar 12, 2015

I have found another windows build incompatibility in copyOwnership in daemon/volumes.go and fixed it.
It now also uses system.GetStat and the system.Stat type.
The change is added to this PR, because it depends on the added GetStat function.

@gewoonrik gewoonrik changed the title from Make utils_daemon cross-platform compileable (Windows). to Make utils_daemon and volumes.go cross-platform compileable (Windows). Mar 12, 2015

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 12, 2015

Collaborator

@gewoonrik why not just use system.Lstat directly?

Collaborator

tiborvass commented Mar 12, 2015

@gewoonrik why not just use system.Lstat directly?

Show outdated Hide outdated daemon/volumes.go
@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 13, 2015

Contributor

Thanks @tiborvass and @cpuguy83, fixed it!

Contributor

gewoonrik commented Mar 13, 2015

Thanks @tiborvass and @cpuguy83, fixed it!

Show outdated Hide outdated utils/utils_daemon.go
@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 13, 2015

Contributor

@tiborvass no problem, shit happens. Done the changes.

Contributor

gewoonrik commented Mar 13, 2015

@tiborvass no problem, shit happens. Done the changes.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 14, 2015

Collaborator

LGTM

Collaborator

tiborvass commented Mar 14, 2015

LGTM

@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft

jhowardmsft Mar 14, 2015

Contributor

This is going to cause a few problems with the refactoring I've done in microsoft/docker for the Windows daemon port where I implement system.stat and system.lstat for Windows. PRs will be in the pipeline for these in the coming week or so.... I'll probably have to be undoing some of these changes with those PRs.

Contributor

jhowardmsft commented Mar 14, 2015

This is going to cause a few problems with the refactoring I've done in microsoft/docker for the Windows daemon port where I implement system.stat and system.lstat for Windows. PRs will be in the pipeline for these in the coming week or so.... I'll probably have to be undoing some of these changes with those PRs.

@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 14, 2015

Contributor

Hmm in that case maybe you should not merge this one.

Contributor

gewoonrik commented Mar 14, 2015

Hmm in that case maybe you should not merge this one.

@tiborvass

This comment has been minimized.

Show comment
Hide comment
@tiborvass

tiborvass Mar 14, 2015

Collaborator

@gewoonrik @jhowardmsft I understand your situation, but usually the rule is: if you don't get your PR merged in time, you get to potentially rebase.

I feel like this PR is small enough to merge (and we encourage contributors to send multiple small patches rather than big ones) and the rebase should not be that painful either.

I'll let @icecrime decide though :)

Collaborator

tiborvass commented Mar 14, 2015

@gewoonrik @jhowardmsft I understand your situation, but usually the rule is: if you don't get your PR merged in time, you get to potentially rebase.

I feel like this PR is small enough to merge (and we encourage contributors to send multiple small patches rather than big ones) and the rebase should not be that painful either.

I'll let @icecrime decide though :)

@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 14, 2015

Contributor

@tiborvass it's ok, I did this PR to be helpful at making Docker Windows compatible, so the last thing I want is getting into the way of @jhowardmsft.

Contributor

gewoonrik commented Mar 14, 2015

@tiborvass it's ok, I did this PR to be helpful at making Docker Windows compatible, so the last thing I want is getting into the way of @jhowardmsft.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Mar 16, 2015

Contributor

Considering @gewoonrik has been actively following on this PR and reactive to people comments, I would be embarrassed to hold it because of upcoming PRs.

LGTM, pushing to 4-merge. Thanks @gewoonrik! And feel free to get in touch with Ahmet on John on IRC if you want to participate in more portability related tasks.

Contributor

icecrime commented Mar 16, 2015

Considering @gewoonrik has been actively following on this PR and reactive to people comments, I would be embarrassed to hold it because of upcoming PRs.

LGTM, pushing to 4-merge. Thanks @gewoonrik! And feel free to get in touch with Ahmet on John on IRC if you want to participate in more portability related tasks.

@icecrime

This comment has been minimized.

Show comment
Hide comment
@icecrime

icecrime Mar 16, 2015

Contributor

@gewoonrik That being said, can you please squash your commits? 😆

Contributor

icecrime commented Mar 16, 2015

@gewoonrik That being said, can you please squash your commits? 😆

Rik Nijessen
Make utils_daemon and volumes cross-platform compileable.
Signed-off-by: Rik Nijessen <riknijessen@gmail.com>
@gewoonrik

This comment has been minimized.

Show comment
Hide comment
@gewoonrik

gewoonrik Mar 16, 2015

Contributor

@icecrime done.

Contributor

gewoonrik commented Mar 16, 2015

@icecrime done.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Mar 16, 2015

Contributor

LGTM

Contributor

LK4D4 commented Mar 16, 2015

LGTM

LK4D4 added a commit that referenced this pull request Mar 16, 2015

Merge pull request #11323 from delftswa2014/utils-daemon-windows
Make utils_daemon and volumes.go cross-platform compileable (Windows).

@LK4D4 LK4D4 merged commit 6485d88 into moby:master Mar 16, 2015

2 checks passed

janky Jenkins build Docker-PRs 3348 has succeeded
Details
windows Jenkins build Windows-PRs 418 has succeeded
Details

@vieux vieux removed the status/4-merge label Mar 16, 2015

@gewoonrik gewoonrik deleted the delftswa2014:utils-daemon-windows branch Mar 16, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment