rootfs: fix compilation error (includes sirupsen->Sirupsen fix) by AkihiroSuda · Pull Request #448 · containerd/containerd

AkihiroSuda · 2017-01-19T06:25:27Z

go build github.com/docker/containerd/rootfs was failing because of an error

can't load package: package github.com/docker/containerd/rootfs: case-insensitive import collision: "github.com/docker/containerd/vendor/github.com/Sirupsen/logrus" and "github.com/docker/containerd/vendor/github.com/sirupsen/logrus"

Ideally we should migrate to sirupsen, but it seems hard. (https://github.com/docker/containerd/pull/415#pullrequestreview-16305753)

So I suggest adapting Sirupsen at the moment.

This PR also fixes a bunch of other compilation errors in rootfs , and some clean up for CLI.

AkihiroSuda · 2017-01-19T06:28:37Z

vendor.conf

Reason of this fix: the go-runc vendored in this PR no longer depends on logrus

AkihiroSuda · 2017-01-19T06:30:20Z

cmd/ctr/run.go

I included this unrelated fix in the PR just because this is nitpicky.
I can open separate PR if I should.

OK, opened https://github.com/docker/containerd/pull/451

estesp · 2017-01-19T16:14:25Z

I know the Sirupsen rename is possibly a problem with far-reaching effects, but I think we need to use the "right" name (lowercase) and start fixing all the imported vendors. I don't think we are in a rush necessarily so even if it takes some time we are OK? Or are we stuck because of the duplicate use (imports using both cases)?

mlaventure · 2017-01-19T16:33:59Z

I agree with @estesp here.

We could fix it in docker/docker by having the vendor script automatically sed'ing Sirupsen to sirupsen for now maybe?

stevvooe · 2017-01-19T22:12:38Z

rootfs/apply.go

Please don't reassign: this fix needs to be upstreamed to OCI. I'll submit a PR today.

I've filed opencontainers/image-spec#514 to take care of this.

hqhq · 2017-01-20T06:43:21Z

I agree we should fix docker and use lowercase version here, that is the right approach.

AkihiroSuda · 2017-01-20T06:52:12Z

Thank you all for the comment.
I agree we should use the lowercase name, and we don't need to rush.

However, IIUC, we should make this package compilable by the end of January, or at the latest, by the end of February.
https://github.com/docker/containerd/milestones

Given that there is a risk we fail to modify docker to use the lowercase name by the period,
WDYT about temporarily forking github.com/docker/docker/pkg/archive to github.com/docker/containerd/archive (and other pkgs depends on Sirupsen if any) and modifying the forked package to use the lowercase name?

When the upstream docker is successfully updated to use the lowercase name, we can safely remove github.com/docker/containerd/archive. (unless there is another reason to fork the package)

mlaventure · 2017-01-20T18:59:34Z

@AkihiroSuda apparently the change to lowercase was reverted (see sirupsen/logrus#451 (comment)), so upper case it is :)

github.com/docker/docker/pkg/archive requires Sirupsen/logrus. So let's remove sirupsen/logrus at the moment. Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda · 2017-01-23T08:53:19Z

@mlaventure
Thank you for the information.

Rebased PR to master.

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

crosbymichael · 2017-01-23T18:09:19Z

LGTM

samuelkarp

LGTM

mlaventure

LGTM

AkihiroSuda mentioned this pull request Jan 19, 2017

rootfs: pseudo-implementation of rootfs unpacking #427

Merged

AkihiroSuda commented Jan 19, 2017

View reviewed changes

vendor.conf Outdated

Copy link

Member Author

AkihiroSuda Jan 19, 2017

Choose a reason for hiding this comment

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

Reason of this fix: the go-runc vendored in this PR no longer depends on logrus

AkihiroSuda commented Jan 19, 2017

View reviewed changes

AkihiroSuda changed the title ~~roots: fix compilation error (includes Sirupsen->sirupsen fix)~~ rootfs: fix compilation error (includes Sirupsen->sirupsen fix) Jan 19, 2017

AkihiroSuda force-pushed the cleanup branch from a3be6c8 to 448aed0 Compare January 19, 2017 07:00

stevvooe reviewed Jan 19, 2017

View reviewed changes

AkihiroSuda changed the title ~~rootfs: fix compilation error (includes Sirupsen->sirupsen fix)~~ rootfs: fix compilation error (includes sirupsen->Sirupsen fix) Jan 20, 2017

vendor: sirupsen/logrus -> Sirupsen/logrus

727f0a5

github.com/docker/docker/pkg/archive requires Sirupsen/logrus. So let's remove sirupsen/logrus at the moment. Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the cleanup branch from 448aed0 to 1a20a04 Compare January 23, 2017 08:52

rootfs: fix compilation error

7fed388

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the cleanup branch from 1a20a04 to 7fed388 Compare January 23, 2017 08:55

samuelkarp approved these changes Jan 23, 2017

View reviewed changes

mlaventure approved these changes Jan 23, 2017

View reviewed changes

mlaventure merged commit 4180eeb into containerd:master Jan 23, 2017

wcarmon mentioned this pull request Apr 19, 2017

Case on https://github.com/sirupsen/logrus import adlio/trello#1

Closed

estesp mentioned this pull request Jul 6, 2017

update to github.com/sirupsen/logrus v1.0.0 #1136

Closed

dmcgowan mentioned this pull request Oct 5, 2023

[release/1.7] Prepare release notes for v1.7.7 #9194

Merged

Conversation

AkihiroSuda commented Jan 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

AkihiroSuda Jan 20, 2017

Choose a reason for hiding this comment

Uh oh!

estesp commented Jan 19, 2017

Uh oh!

mlaventure commented Jan 19, 2017

Uh oh!

stevvooe Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe Jan 19, 2017

Choose a reason for hiding this comment

Uh oh!

hqhq commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkihiroSuda commented Jan 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mlaventure commented Jan 20, 2017

Uh oh!

AkihiroSuda commented Jan 23, 2017

Uh oh!

crosbymichael commented Jan 23, 2017

Uh oh!

samuelkarp left a comment

Choose a reason for hiding this comment

Uh oh!

mlaventure left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

AkihiroSuda commented Jan 19, 2017 •

edited

Loading

hqhq commented Jan 20, 2017 •

edited

Loading

AkihiroSuda commented Jan 20, 2017 •

edited

Loading