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

Vendor in containers/[email protected] #14127

Merged
merged 7 commits into from
May 5, 2022

Conversation

rhatdan
Copy link
Member

@rhatdan rhatdan commented May 5, 2022

Does this PR introduce a user-facing change?


@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 5, 2022
@rhatdan
Copy link
Member Author

rhatdan commented May 5, 2022

@flouthoc @Luap99 @vrothberg @giuseppe PTAL
@containers/podman-maintainers PTAL

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

LGTM but we should fill out the change-log field

Copy link
Collaborator

@flouthoc flouthoc left a comment

Choose a reason for hiding this comment

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

LGTM

@flouthoc
Copy link
Collaborator

flouthoc commented May 5, 2022

@rhatdan Build Each Commit is failing with conflicts does it need a fresh make vendor PR instead of cherry-pick ?

@rhatdan
Copy link
Member Author

rhatdan commented May 5, 2022

@flouthoc Weird, I don't get any errors when I make vendor, could you pull this on your machine and see if you see errors?

@vrothberg
Copy link
Member

I doesn't look like a vendor fart to me but something happens during a rebase:

Executing: hack/make-and-check-size /tmp/make-size-check.mOEtXud
Building: 7a0a3adc8 Add 4.1 branch to API documentation
make[1]: Entering directory '/var/tmp/go/src/github.com/containers/podman'
CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build \
	-mod=vendor  \
	-ldflags '-X github.com/containers/podman/v4/libpod/define.gitCommit=7a0a3adc83c2886d88a8c1a4795107417d8802ee -X github.com/containers/podman/v4/libpod/define.buildInfo=1651752017 -X github.com/containers/podman/v4/libpod/config._installPrefix=/usr/local -X github.com/containers/podman/v4/libpod/config._etcDir=/usr/local/etc -X github.com/containers/common/pkg/config.additionalHelperBinariesDir= ' \
	-tags "remote exclude_graphdriver_btrfs btrfs_noversion exclude_graphdriver_devicemapper containers_image_openpgp" \
	-o bin/podman-remote ./cmd/podman
if [ ! -x "/usr/bin/go-md2man" ]; then \
	make -C test/tools build/go-md2man ; \
fi
mkdir -p docs/build/man
make[1]: Leaving directory '/var/tmp/go/src/github.com/containers/podman'
bin/podman           size= 47609672  delta=     0
bin/podman-remote    size= 35758104  delta=     0
bin/rootlessport     size=  4889248  delta=     0
Rebasing (19/22)
Auto-merging go.mod
CONFLICT (content): Merge conflict in go.mod
Auto-merging go.sum
CONFLICT (content): Merge conflict in go.sum
Auto-merging vendor/modules.txt
CONFLICT (content): Merge conflict in vendor/modules.txt
error: could not apply 9ca88876f... Vendor in containers/[email protected]
hint: Resolve all conflicts manually, mark them as resolved with
hint: "git add/rm <conflicted_files>", then run "git rebase --continue".
hint: You can instead skip this commit: run "git rebase --skip".
hint: To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply 9ca88876f... Vendor in containers/[email protected]
make: *** [Makefile:294: build-all-new-commits] Error 1

I do not yet see why though.

@Luap99
Copy link
Member

Luap99 commented May 5, 2022

From the log at the top:

# Validate that all the commits build on top of origin/main^
git rebase origin/main^ -x "hack/make-and-check-size /tmp/make-size-check.mOEtXud"

Why would it base on origin/main? It should base it of origin/v4.1.

@vrothberg
Copy link
Member

@rhatdan, apply the following patch:

diff --git a/Makefile b/Makefile
index caa991b140d5..bf60d26c8f91 100644
--- a/Makefile
+++ b/Makefile
@@ -32,7 +32,7 @@ HEAD ?= HEAD
 CHANGELOG_BASE ?= HEAD~
 CHANGELOG_TARGET ?= HEAD
 PROJECT := github.com/containers/podman
-GIT_BASE_BRANCH ?= origin/main
+GIT_BASE_BRANCH ?= origin/v4.1
 GIT_BRANCH ?= $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null)
 GIT_BRANCH_CLEAN ?= $(shell echo $(GIT_BRANCH) | sed -e "s/[^[:alnum:]]/-/g")
 LIBPOD_INSTANCE := libpod_dev

@vrothberg
Copy link
Member

Why would it base on origin/main? It should base it of origin/v4.1.

Yup, that's it. It failed in make build-all-new-commits.

@Luap99
Copy link
Member

Luap99 commented May 5, 2022

@rhatdan, apply the following patch:

diff --git a/Makefile b/Makefile
index caa991b140d5..bf60d26c8f91 100644
--- a/Makefile
+++ b/Makefile
@@ -32,7 +32,7 @@ HEAD ?= HEAD
 CHANGELOG_BASE ?= HEAD~
 CHANGELOG_TARGET ?= HEAD
 PROJECT := github.com/containers/podman
-GIT_BASE_BRANCH ?= origin/main
+GIT_BASE_BRANCH ?= origin/v4.1
 GIT_BRANCH ?= $(shell git rev-parse --abbrev-ref HEAD 2>/dev/null)
 GIT_BRANCH_CLEAN ?= $(shell echo $(GIT_BRANCH) | sed -e "s/[^[:alnum:]]/-/g")
 LIBPOD_INSTANCE := libpod_dev

Note sure if this is the right solution, looks like DEST_BRANCH is not set correctly

DEST_BRANCH: "main"

GIT_BASE_BRANCH=origin/"${DEST_BRANCH}^" \

go.mod Outdated
github.com/containers/image/v5 v5.21.1-0.20220425080628-be085685e524
github.com/containers/ocicrypt v1.1.3
github.com/containers/image/v5 v5.21.1
github.com/containers/ocicrypt v1.1.4-0.20220428134531-566b808bdf6f
Copy link
Member

Choose a reason for hiding this comment

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

It bothers me to vendor a non-release. I'll check where this comes from.

Copy link
Member

Choose a reason for hiding this comment

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

@mheon
Copy link
Member

mheon commented May 5, 2022

@Luap99 I'm not sure if that's it, the 4.0 branch still has that set to main

@Luap99
Copy link
Member

Luap99 commented May 5, 2022

@Luap99 I'm not sure if that's it, the 4.0 branch still has that set to main

Yes and that is also wrong?! The bloat check was only added recently and I think this is causing the issue.

@mheon
Copy link
Member

mheon commented May 5, 2022

Fair enough, seems reasonable to try.

@rhatdan Can you repush with the requested changes, or should I take this over?

@rhatdan rhatdan force-pushed the v4.1 branch 2 times, most recently from cd7f607 to 3e5e0d2 Compare May 5, 2022 16:44
@Luap99 Luap99 added the bloat_approved Approve a PR in which binary file size grows by over 50k label May 5, 2022
cevich added 4 commits May 5, 2022 13:45
Now that netavark and aardvark are packaged and default in F36, support
CNI-based testing in F35 and Ubuntu.

* Remove the temporary/special `$TEST_ENVIRON=host-netavark` construct.
* Remove dedicated/special integration and system testing tasks.
* Update test-config setup to properly handle CNI vs netavark/aardvark
  environments.
* Update package-version logging to operate based on installed packages
  (along with some other minor script cleanups).
* Update global environment setup to force `$NETWORK_BACKEND=netavark`
  in F36 and later.  Except when `upgrade_test` task runs.
* Discontinue installing netavark and aardvark-dns binaries from
  upstream build artifacts.
* Drop CGV1-vs-2 policy check.  Ubuntu VMs now exclusively test CGv1,
  Fedora VMs test CGv2, with F35 testing CNI and F36 testing Netavark.

Signed-off-by: Chris Evich <[email protected]>
Normally installing/updating packages at test runtime is highly
discouraged for reliability and efficiency reasons.  However, in this
specific case, development work of these packages is still fairly hot.
As a compromise to support podman test development, temporarily update
these two specific packages at runtime.  At a future date, when updates
are less frequent, this commit can/should be safely reverted.  At that
point, the versions installed at VM image build time will persist.

Signed-off-by: Chris Evich <[email protected]>
rhatdan and others added 3 commits May 5, 2022 13:52
Disable `build --output` for remote clients and update docs.

[NO NEW TESTS NEEDED]
[NO TESTS NEEDED]

Signed-off-by: Aditya R <[email protected]>
Signed-off-by: Daniel J Walsh <[email protected]>
Newer versions of git are much more pedantic about who owns the
repository files.  When setting up to run rootless, prior to this
commit, the repo. ownership was changed from root.  This causes
all subsequent git-operations as root to fail:

    ```
    fatal: unsafe repository ('<$GOSRC>' is owned by someone else)
    ```

Fix this by re-ordering operations, such that the change in ownership is
done immediately before executing as a user.  Also disable the
git-ownership check on the source repository assuming the CI environment
is disposable.

Signed-off-by: Chris Evich <[email protected]>
@mheon
Copy link
Member

mheon commented May 5, 2022

I'm going to
/lgtm
/approve
/hold

So I can quickly get 4.1.0 final out

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2022
@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 5, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc, mheon, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [flouthoc,mheon,rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@rhatdan
Copy link
Member Author

rhatdan commented May 5, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 5, 2022
@openshift-merge-robot openshift-merge-robot merged commit 83ef2c7 into containers:v4.1 May 5, 2022
edsantiago added a commit to edsantiago/libpod that referenced this pull request May 12, 2022
Buildah got vendored into podman last week, and the script
went kablooie because of ever-so-slight conflicts between
what was in the treadmill PR (containers#13808) and what ultimately
got merged (containers#14127) which was obviously better (hey, I tried).

After a buildah vendor, there really isn't any point to keeping
the treadmill commits - we're much better off just restarting
with two fresh empty placeholder commits. Do so.

Also, mild cleanup.

Signed-off-by: Ed Santiago <[email protected]>
cdoern pushed a commit to cdoern/podman that referenced this pull request May 27, 2022
Buildah got vendored into podman last week, and the script
went kablooie because of ever-so-slight conflicts between
what was in the treadmill PR (containers#13808) and what ultimately
got merged (containers#14127) which was obviously better (hey, I tried).

After a buildah vendor, there really isn't any point to keeping
the treadmill commits - we're much better off just restarting
with two fresh empty placeholder commits. Do so.

Also, mild cleanup.

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bloat_approved Approve a PR in which binary file size grows by over 50k lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants