bddeb: new --packaging-branch argument to pull packaging from branch #576

paride · 2020-09-16T15:57:13Z

bddeb builds a .deb package using the template packaging files in
packages/debian/.

The new --packaging-branch flag allows to specify a git branch
where to pull the packaging (i.e. the debian/ directory) from.
This is useful to build a .deb package from master with the very
same packaging which is used for the uploads.

paride · 2020-09-16T16:31:39Z

I was temped to propose this as the new default build method, but the "templated" approach still has a couple of advantages:

doesn't require a packaging branch
auto generates the package dependencies via read-dependencies. Not sure this is a real advantage as I want to see the build fail if there's a missing dependency...

and possibly other things I'm missing, so I'm leaving the default alone, unless the team thinks otherwise.

raharper

I really like this change. For older release branches (like xenial) it's much harder to test correctly since bddeb won't have many of the per-branch changes to the runtime code (not just packing changes).

Some comments in-line

packages/bddeb

paride · 2020-09-17T09:29:25Z

Thanks for the comments @raharper, I updated the branch. Notable changes:

more use of subp()
check if debian/control exists in the specified branch, per your suggestion
renamed --packaging-from-branch to --packaging-branch
exclude debian/patches when importing the packaging

I'd like to have some feedback on the debian/patches exclusion. Seems a good idea as bddeb is mainly used from master, and as master moves patches do not apply anymore, and the build breaks. And by default (i.e. without --packaging-branch) bddeb doesn't even know about patches. However when building from release branches it would be nice to include the patches. I thought about not excluding the patches if pakaging_branch == current_branch, but this makes the script behavior less obvious. Thoughts?

raharper · 2020-09-17T14:56:39Z

I'd like to have some feedback on the debian/patches exclusion. Seems a good idea as bddeb is mainly used from master, and as master moves patches do not apply anymore, and the build breaks. And by default (i.e. without --packaging-branch) bddeb doesn't even know about patches. However when building from release branches it would be nice to include the patches. I thought about not excluding the patches if pakaging_branch == current_branch, but this makes the script behavior less obvious. Thoughts?

I very much would expect patches to come as well; otherwise I'd just build master. You're correct that building master with merged release branch and patches won't always work. However, our daily recipes already have to deal with this; IIUC, there is a two step branch merge to address this. @blackboxsw and @OddBloke have this worked out for our daily recipe; so we'd need to pull that in.

Given that it's more complicated, but desirable (IMO) then I think this could use a --without-patches to disable this behavior.

paride · 2020-09-17T15:45:39Z

+1 on keeping the patches.

If I connected the dots correctly correctly, the daily recipe reverts the cherry-picked patches, which are identified by their filename (has to match debian/patches/*cpick*). We could exclude the debian/patches/*cpick* files from the debian/ import, keeping all the other patches.

paride · 2020-09-18T08:53:24Z

That's probably not a good idea either.

I dropped the "exclude patches" bit, let's wait and see if we hit an actual situation where we would like some patches to be excluded and then see if there's a sensible way to do it.

paride · 2020-09-23T12:35:07Z

Rebased on master.

paride · 2020-09-23T16:10:26Z

hey @raharper, does the current PR address your review suggestions?

cloudinit/subp.py

packages/bddeb

raharper · 2020-09-23T16:25:58Z

packages/bddeb

+
+    print("Adding new entry to debian/changelog")
+    full_deb_version = (
+        templ_data["version_long"] + "-1~bddeb" + templ_data["release_suffix"]


Did we close on whether this value ("-1~bddeb") needs to be user-controllable?

Like this is behaves like plain bddeb without --packaging-branch. We can certainly make it configurable, but is it useful in practice?

github-actions · 2020-10-16T00:28:34Z

Hello! Thank you for this proposed change to cloud-init. This pull request is now marked as stale as it has not seen any activity in 14 days. If no activity occurs within the next 7 days, this pull request will automatically close.

If you are waiting for code review and you are seeing this message, apologies! Please reply, tagging mitechie, and he will ensure that someone takes a look soon.

(If the pull request is closed, please do feel free to reopen it if you wish to continue working on it.)

blackboxsw · 2020-10-17T02:41:53Z

That's probably not a good idea either.

I dropped the "exclude patches" bit, let's wait and see if we hit an actual situation where we would like some patches to be excluded and then see if there's a sensible way to do it.

I think that's the best option @paride. We will have this case any time we have a "cpicked" patch back into an ubunut/. We'll need to pop that cpick file off the stack if we want to continue to build using ubuntu/xenial/debian on master. I think this case is unneeded though as this complexity is solved by the ubuntu/daily/ branches and doesn't really need to be solved by your switch here. I think the majority of cases we would use this in would be just building master with ubuntu/devel/debian dir, and the devel branch shouldn't really carry cherry picks as we would mostly prefer to just release tip of master into ubuntu/devel for two reasons:

master is almost always releasable
we only cherry-pick into ubuntu/devel during devel series feature-freeze events once every 6 months.

I don't think it warrants us designing the cpick* patch-popping algorithm in bddeb as we don't use bddeb in the daily recipes. We instead rely on a simplified package branching strategy to allow our daily builds to succeed when merging tip of master into the ubuntu/ branch.

packages/bddeb

bddeb normally builds a .deb package using the template packaging files in packages/debian/. The new --packaging-branch argument allows to specify a git branch where to pull the packaging (i.e. the debian/ directory) from. This is useful to build a .deb package from master with the very same packaging which is used for the uploads. The contents of debian/patches is excluded.

Co-authored-by: Ryan Harper <rharper@woxford.com>

If the packaging branch is missing hint the user to do a local checkout of the branch.

paride · 2020-10-19T15:59:27Z

Thanks for the review, @blackboxsw hint added.

I agree ubuntu/devel is the the branch that --packaging-branch is meant to target most of the time, and it's indeed the default, and we shouldn't normally carry cherry-picks there for the reasons you stated.

confirmed we are ok on this current branch via IRC conversation

paride force-pushed the bddeb-from-branch branch from d7cabc9 to 0130f93 Compare September 16, 2020 16:10

raharper reviewed Sep 16, 2020

View reviewed changes

packages/bddeb Show resolved Hide resolved

paride force-pushed the bddeb-from-branch branch from 0130f93 to 4ed6cd8 Compare September 17, 2020 09:19

paride force-pushed the bddeb-from-branch branch from 4ed6cd8 to e0e13ac Compare September 17, 2020 09:37

paride force-pushed the bddeb-from-branch branch from b166325 to 98c4ce8 Compare September 18, 2020 08:53

paride changed the title ~~bddeb: add --packaging-from-branch argument~~ bddeb: new --packaging-branch argument to pull packaging from branch Sep 18, 2020

paride force-pushed the bddeb-from-branch branch from 98c4ce8 to f672d8b Compare September 23, 2020 12:29

raharper previously requested changes Sep 23, 2020

View reviewed changes

TheRealFalcon assigned paride Oct 1, 2020

github-actions bot added the stale-pr Pull request is stale; will be auto-closed soon label Oct 16, 2020

paride removed the stale-pr Pull request is stale; will be auto-closed soon label Oct 16, 2020

blackboxsw reviewed Oct 19, 2020

View reviewed changes

packages/bddeb Show resolved Hide resolved

paride and others added 9 commits October 19, 2020 17:53

subp: add a cwd argument to execute commands in a different directory

b5fa237

Document bddeb --packaging-branch

f988730

bddeb --packaging-branch: do not exclude patches

52c8382

bddeb --packaging-branch: be more verbose when importing packaging

4b0b5ca

Fix typo in cloudinit/subp.py

cfeff88

Co-authored-by: Ryan Harper <rharper@woxford.com>

write_debian_folder_from_branch: fix format string

f93c1a0

Co-authored-by: Ryan Harper <rharper@woxford.com>

Drop unrelated newline

e5ab46f

bddeb: give hint on checking out the packaging branch

71a972b

If the packaging branch is missing hint the user to do a local checkout of the branch.

paride force-pushed the bddeb-from-branch branch from 0535c18 to 71a972b Compare October 19, 2020 15:53

paride requested a review from blackboxsw October 19, 2020 15:59

blackboxsw approved these changes Oct 19, 2020

View reviewed changes

Merge branch 'master' into bddeb-from-branch

868cefc

blackboxsw merged commit 5a7f681 into canonical:master Oct 19, 2020

This was referenced May 12, 2023

cloud-init regenerating ssh-keys #3753

Closed

Release 20.4 #3811

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bddeb: new --packaging-branch argument to pull packaging from branch #576

bddeb: new --packaging-branch argument to pull packaging from branch #576

paride commented Sep 16, 2020 •

edited

Loading

paride commented Sep 16, 2020 •

edited

Loading

raharper left a comment

paride commented Sep 17, 2020

raharper commented Sep 17, 2020

paride commented Sep 17, 2020 •

edited

Loading

paride commented Sep 18, 2020 •

edited

Loading

paride commented Sep 23, 2020

paride commented Sep 23, 2020

raharper Sep 23, 2020

paride Sep 23, 2020

github-actions bot commented Oct 16, 2020

blackboxsw commented Oct 17, 2020

paride commented Oct 19, 2020

bddeb: new --packaging-branch argument to pull packaging from branch #576

bddeb: new --packaging-branch argument to pull packaging from branch #576

Conversation

paride commented Sep 16, 2020 • edited Loading

paride commented Sep 16, 2020 • edited Loading

raharper left a comment

Choose a reason for hiding this comment

paride commented Sep 17, 2020

raharper commented Sep 17, 2020

paride commented Sep 17, 2020 • edited Loading

paride commented Sep 18, 2020 • edited Loading

paride commented Sep 23, 2020

paride commented Sep 23, 2020

raharper Sep 23, 2020

Choose a reason for hiding this comment

paride Sep 23, 2020

Choose a reason for hiding this comment

github-actions bot commented Oct 16, 2020

blackboxsw commented Oct 17, 2020

paride commented Oct 19, 2020

paride commented Sep 16, 2020 •

edited

Loading

paride commented Sep 16, 2020 •

edited

Loading

paride commented Sep 17, 2020 •

edited

Loading

paride commented Sep 18, 2020 •

edited

Loading