Skip to content

libcontainer: rm windows pieces#2700

Closed
kolyshkin wants to merge 3 commits intoopencontainers:masterfrom
kolyshkin:rm-win
Closed

libcontainer: rm windows pieces#2700
kolyshkin wants to merge 3 commits intoopencontainers:masterfrom
kolyshkin:rm-win

Conversation

@kolyshkin
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin commented Dec 3, 2020

(this used to be part of #2695)

This removes some Windows-only code from libcontainer. See individual commits for details.

@kolyshkin kolyshkin mentioned this pull request Dec 3, 2020
Merged
mrunalp
mrunalp previously approved these changes Dec 4, 2020
View reviewed changes
@kolyshkin kolyshkin changed the title libcontainer: rm windows pieces [DO NOT MERGE] libcontainer: rm windows pieces Dec 4, 2020
@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Dec 15, 2020

For context, this is DO NOT MERGE since @thaJeztah is currently looking into who's using this from Windows code. Once the Windows users are removed, this can be merged.

@kolyshkin
libcontainer/configs/config_windows_test.go: rm
c8cecf8 
Nothing is in there, so removing.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libcontainer/devices/device_windows.go: rm
4b3e120 
This was initially added by commit d78ee47 but later
moved from libcontainer/configs to libcontainer/devices by
commit 677baf2.

Looks like since commit 677baf2 and also [1]
there is no use for this, thus removing.

[1] containers/buildah#2652

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
libct/user: rm windows code
db71bce 
Commit bf74951 added these two functions, but they are only used from
Windows code. The v1 of this patch moved these functions to _windows.go
file, but after some discussion we decided to drop windows code
altogether, so this is what this patch now does.

This fixes

> libcontainer/user/user.go:64:6: func `groupFromOS` is unused (unused)
> libcontainer/user/user.go:35:6: func `userFromOS` is unused (unused)

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Jan 6, 2021

rebased (for new CI)

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Jan 14, 2021

Is this still DNM?

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Jan 14, 2021

Ah, keep forgetting; I should try vendoring from this branch to see if (and where) things break

@kolyshkin kolyshkin mentioned this pull request Feb 1, 2021
Closed
@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Feb 24, 2021

Once the Windows users are removed, this can be merged.

Can we merge this right now?
We don't guarantee Go compatibility of libcontainer. If something no longer compiles on Windows, the maintainers of that project just copy the deleted codes to their project.

@AkihiroSuda AkihiroSuda reopened this Feb 24, 2021
@AkihiroSuda AkihiroSuda added this to the 1.0.0-rc94 milestone Feb 24, 2021
@kolyshkin kolyshkin changed the title [DO NOT MERGE] libcontainer: rm windows pieces libcontainer: rm windows pieces Feb 26, 2021
@kolyshkin
Copy link
Copy Markdown
Contributor Author

kolyshkin commented Feb 26, 2021

I'm fine with merging this, but we need to hear back from @thaJeztah as we don't want to break Docker

thaJeztah added a commit to thaJeztah/cli that referenced this pull request Mar 13, 2021
@thaJeztah
Test libcontainer changes (removal of Windows code)
9e9df85 
testing if things break with opencontainers/runc#2700

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Mar 13, 2021
Closed
@thaJeztah thaJeztah mentioned this pull request Mar 13, 2021
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Mar 13, 2021

Ah, sorry, forgot about this one; I think we should be fine, but opened docker/cli#3011 and moby/moby#42143, which vendor from this branch to see if things break

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Mar 14, 2021

@kolyshkin I opened kolyshkin#9 against your branch with some fixes that were needed to make it work (moby/moby#42143). Also opened #2851 with other changes related to that

@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Mar 22, 2021

@kolyshkin I opened kolyshkin#9 against your branch with some fixes that were needed to make it work (moby/moby#42143). Also opened #2851 with other changes related to that

@kolyshkin could you have a look at kolyshkin#9 ? ^^

@thaJeztah thaJeztah mentioned this pull request Apr 2, 2021
Closed
@thaJeztah thaJeztah mentioned this pull request Apr 2, 2021
Merged
@thaJeztah
Copy link
Copy Markdown
Member

thaJeztah commented Apr 2, 2021

I rebased this one and included my patch from kolyshkin#9 in #2888

@cyphar
Copy link
Copy Markdown
Member

cyphar commented Apr 3, 2021

I'll close this in favour of #2888.

@cyphar cyphar closed this Apr 3, 2021
kolyshkin added a commit that referenced this pull request Apr 4, 2021
@kolyshkin
Merge pull request #2888 from thaJeztah/fixup_rm_win_carry
365c628 
libcontainer: rm windows pieces (carry #2700)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrunalp mrunalp mrunalp left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

1.0.0-rc94

Development

Successfully merging this pull request may close these issues.

5 participants

@kolyshkin @AkihiroSuda @thaJeztah @cyphar @mrunalp