Paths

Table of Contentst

UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8)
ClosedPublic
Actions

Authored by driesm on Jan 9 2021, 1:57 PM.

Details

Reviewers

ngie
bcr
imp
debdrup
ceri
philip
0mp

Group Reviewers

docs

Commits

rGe641c29a006a: UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8)

Summary

This changes the procedure in UPDATING to use etcupdate over megemaster.
This follows the recommendations in the handbook commited from D27848.

Commit message:

UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8)

This commit aligns the steps in UPDATING with the steps from the handbook which already prefers etcupdate(8). While here also remove a dubious comment.

 PR: 252417
 Reviewed by: 0mp, ceri, imp
 Approved by: philip (mentor)
 Differential Revision: https://reviews.freebsd.org/D28062
`

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 38810
Build 35699: arc lint + arc unit

Event Timeline

driesm requested review of this revision.Jan 9 2021, 1:57 PM

driesm created this revision.

Harbormaster completed remote builds in B36059: Diff 81951.Jan 9 2021, 1:58 PM

Could you please add PR: 252417 to the commit message?

UPDATING
2403	It never hurts to do it all the time. This advice can actually break a system if the new etcupdate relies on behavior/code from the newly built world. Please remove this comment or add similar advice to the original advice, noting the potential pitfall of using etcupdate(8), which can be incompatible with the old installed world.
2403	This was also the case for mergemaster too. Oof... that was bad advice.

Thanks for the review! I don't have a bit but I have updated the description with the PR number.

UPDATING
2403	So the comment that was there before is also applicable for worlds after 20130425 and 20130430? As the message only hints for the pitfall if we are updating from before these revisions (the way I interpret it, possibly incorrect though). Or is it a possible pitfall in general? If so I would just drop the specific revisions mentioned there as its confusing.

driesm edited the summary of this revision. (Show Details)Jan 9 2021, 3:17 PM

Incorporate feedback.

driesm marked 2 inline comments as done.Jan 13 2021, 8:20 PM

Harbormaster completed remote builds in B36177: Diff 82225.Jan 13 2021, 8:21 PM

driesm added a reviewer: docs.Mar 17 2021, 7:58 PM

My eyeballs tell me these changes are good, but I've not 'play tested' them in anger to be sure.

UPDATING
2408	This change is independent, but imho, not important enough to do as a separate commit.

This revision is now accepted and ready to land.Mar 17 2021, 8:21 PM

Hi @debdrup, as you landed my review for updating the handbook to reference etcupdate, do you mind taking this one too? Thanks

grahamperrin added a subscriber: grahamperrin.Apr 12 2021, 1:30 AM

grahamperrin added inline comments.

UPDATING
2315	Move up, to be inline.
2316	Remove white space.
2353	Move up, to be inline.
2354	Remove white space.
2358	I know that deleting old libraries can cause breakage (if performed carelessly). Is there also risk of breakage for `delete-old` – without attention to libraries? I routinely: `make -DBATCH_DELETE_OLD_FILES delete-old` – and never encountered breakage. Touch wood.
2383	Take this opportunity to renumber 3, 4, 5, 6, 8 and 9, and the referring points? (There is no footnote 2 or 7.) Then – order – begin the points, and their footnotes, at 1 instead of 7.
2429	Add white space between this line and the subheading that follows.
2440	1998–2021

imp added inline comments.Apr 12 2021, 1:37 AM

UPDATING
2440	I've not been the majority contributor since 2009.

grahamperrin added inline comments.Apr 22 2021, 6:23 AM

UPDATING
2283	if there is no EFI system partition

No real change intended rebased to master.

This revision now requires review to proceed.Apr 22 2021, 6:46 PM

Harbormaster completed remote builds in B38810: Diff 88006.Apr 22 2021, 6:46 PM

In general your comments are valid and will open different reviews for some of them. But I'd like to keep this one just simple to change the mergemaster commands to etcupdate.

UPDATING
2315	It is intended like this so that it is not associated with make installkernel, it is seen as a seperate step between installkernel and reboot to single user mode.
2358	I'm trying to focus this review on mergemsater => Etcupdate. I've already folded in one unrelated change. I hope you understand.
2383	I'm trying to focus this review on mergemsater => Etcupdate. I've already folded in one unrelated change. I hope you understand.

In D28062#671670, @driesm.michiels_gmail.com wrote:

… keep this one just simple to change the mergemaster commands to etcupdate.

Understood and agreed; https://reviews.freebsd.org/D29934 noted with thanks.

Hi all, could this regain some traction?
The conflicting information between the handbook and the notes in UPDATING is confusing a few people on the forums etc.

Change looks good, I think, but is there any reason not to renumber those "footnotes" at the same time so that they are vaguely in order and 2 and 7 aren't missing for no good reason?

In D28062#690276, @ceri wrote:

Change looks good, I think, but is there any reason not to renumber those "footnotes" at the same time so that they are vaguely in order and 2 and 7 aren't missing for no good reason?

I try to keep the reviews small so they are easier to review. I made a separate one for that.
And yes, it is way better after reordering :-) https://reviews.freebsd.org/D29934

ceri accepted this revision.Jun 11 2021, 4:06 PM

This revision is now accepted and ready to land.Jun 11 2021, 4:06 PM

I don't really have time for this right now, so someone else can go ahead and commit it once it's been accepted.

In D28062#690713, @debdrup wrote:

I don't really have time for this right now, so someone else can go ahead and commit it once it's been accepted.

@ngie - you’re holding the PR; will you commit?

Hi ceri, Warner accept a previous version, is that enough? The patch hasn't changed since then, I just rebased, content remains the same.

I accept this version two: I just have a quick comment about updating EFI boot that needn't be addressed in this.

UPDATING
2283	No. **N E V E R ** use gpart bootcode on a UEFI system. It's always wrong now.

Anyone taking care of the commit? The handbook and UPDATING still give different step by step ATM ...

driesm edited the summary of this revision. (Show Details)Nov 9 2021, 2:48 PM

driesm added reviewers: philip, 0mp.

As I now have some (limited) power of my own I'd like to land this with the necessary approvals.

Given this is a change to the src repo, I'll need an approval from someone who contains a src commit bit, if I read this right in the committers guide :-).
Philip is this something you can provide? Technically, imp already accepted as well. Do I put both of you in the Approved by field?

Thanks for the feedback!

driesm retitled this revision from Change procedure in UPDATING to use etcupdate(8) over mergemaster(8) to UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8).Nov 9 2021, 2:55 PM

driesm edited the summary of this revision. (Show Details)

I'll reiterate that the green/red changes look good.
I want more, of course, but better is better, right?
tl;dr: go for it with my blessing.

driesm edited the summary of this revision. (Show Details)Nov 9 2021, 10:03 PM

Approved by: philip (mentor)

Closed by commit rGe641c29a006a: UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8) (authored by driesm). · Explain WhyNov 10 2021, 8:28 AM

This revision was automatically updated to reflect the committed changes.

driesm added a commit: rGe641c29a006a: UPDATING: Change update procedure to use etcupdate(8) over mergemaster(8).