Skip to content

replace sys Sequential funcs with moby/sys/sequential#7334

Merged
kzys merged 1 commit intocontainerd:mainfrom
thaJeztah:sequential
Aug 30, 2022
Merged

replace sys Sequential funcs with moby/sys/sequential#7334
kzys merged 1 commit intocontainerd:mainfrom
thaJeztah:sequential

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

These functions were originally copied from the docker / moby repository in 4a7a8ef (#415). Migrating these functions to use the github.com/moby/sys/sequential module allows them being shared between moby, docker/cli, and containerd, and to allow using them without importing all of sys which also depends on hcsshim and more.

@dims
Copy link
Copy Markdown
Member

dims commented Aug 27, 2022

"without importing all of sys" ... Nice!!! ❤️

@thaJeztah
Copy link
Copy Markdown
Member Author

You may like #7335 as well :)

@thaJeztah
Copy link
Copy Markdown
Member Author

For reviewers looking at the "+ LOC"; most of the added lines from the new module is because

  • it'll vendor another copy of the Apache LICENSE
  • because of the (temporary) "deprecated" stubs; those should be fine to remove after the next release (giving users time to transition)
  • the "moby" code currently depends on the stubs for Unix (the containerd code only used these on Windows), which is mostly boilerplating, and won't be used in the code here (I guess go modules could be smarter when vendoring, and prune "other" architectures; I guess it doesn't to allow for the vendored code to be tested).

@thaJeztah thaJeztah changed the title [WIP] replace sys Sequential funcs with moby/sys/sequential replace sys Sequential funcs with moby/sys/sequential Aug 29, 2022
Comment thread go.mod
github.com/klauspost/compress v1.15.9
github.com/moby/locker v1.0.1
github.com/moby/sys/mountinfo v0.6.2
github.com/moby/sys/sequential v0.0.0-20220829095930-b22ba8a69b30
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No release yet, but I'll do a release once review of all PR's is looking good, and will update to v0.5.0 (once release) in a follow-up.

@thaJeztah
Copy link
Copy Markdown
Member Author

@dims @AkihiroSuda this one should be ready for review 👍

Copy link
Copy Markdown
Member

@dims dims left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread archive/tar_windows.go Outdated
Comment on lines 66 to 67
// Source is regular file. We use sys.OpenFileSequential to use sequential
// file access to avoid depleting the standby list on Windows.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I should probably update the comments as well accordingly; let me fix that.

These functions were originally copied from the docker / moby repository in
4a7a8ef. Migrating these functions to use the
github.com/moby/sys/sequential module allows them being shared between moby,
docker/cli, and containerd, and to allow using them without importing all of sys
which also depends on hcsshim and more.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah
Copy link
Copy Markdown
Member Author

Let me try close/reopen to give CI a kick

@thaJeztah thaJeztah closed this Aug 29, 2022
@thaJeztah thaJeztah reopened this Aug 29, 2022
Copy link
Copy Markdown
Member

@kzys kzys left a comment

Choose a reason for hiding this comment

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

LGTM. Re-running failed tests.

@kzys kzys merged commit 4032aed into containerd:main Aug 30, 2022
@thaJeztah thaJeztah deleted the sequential branch August 30, 2022 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants