Skip to content
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

remove remap-rootfs bin when running make clean #4139

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

lifubang
Copy link
Member

@lifubang lifubang commented Dec 8, 2023

I have reviewed and found such errors for about 2 times.
So I think move the target clean next to all will reduce such type error in the future?

@lifubang lifubang added easy-to-review backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 labels Dec 8, 2023
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM, though I wonder if it would be better to have a per-contrib-cmd Makefile (and move the .gitignore entries while we're at it). Then again, we configure the go build line fairly heavily so maybe that wouldn't work too well...

Comment on lines -189 to -190
.PHONY: clean
clean:
Copy link
Member

Choose a reason for hiding this comment

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

Was there a reason you moved this to a different location in the Make file? (it makes it harder to see what changed)

Copy link
Member

Choose a reason for hiding this comment

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

I think the idea is to make it harder to miss when we add new targets in the future:

So I think move the target clean next to all will reduce such type error in the future?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, yeah, probably; was considering 2 commits (one that moves it, and one that updates it), but not a hard blocker.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!
Another think, as @kolyshkin asked in #4140 (comment) , I think we should make one more 1.1.x release because of the issue #4122 . But it's very hard to backport #4124 to release-1.1 , because in this PR, there are two topics in one commit, while one topic of them is not in release-1.1. @cyphar

Copy link
Member

Choose a reason for hiding this comment

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

That backport is ready in #4144.

@lifubang lifubang added this to the 1.1.11 milestone Dec 13, 2023
@lifubang lifubang removed the backport/todo/1.1 A PR in main branch which needs to be backported to release-1.1 label Dec 14, 2023
@lifubang lifubang removed this from the 1.1.11 milestone Dec 14, 2023
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
Signed-off-by: lfbzhm <lifubang@acmcoder.com>
Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

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

LGTM, though I think splitting the makefile might make more sense (we can do that in a future PR).

@AkihiroSuda AkihiroSuda merged commit 827c707 into opencontainers:main Jan 4, 2024
45 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants