Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Update to complete patch, this fixes leftovers for all knobs from src.conf(5) (except for WITHOUT_CXX, which deliberately leaves devd, see comment).
I'll review this piece by piece. Please commit each block that I approve so we can "whittle" down the diff.
Ok, I've assumed the process will be something like that. I propose to commit moves (SVN, DMAGENT, HYPERV) first, then completely new chunks.
- Please don't remove any OLD_DIRS directives until the bits you have have been pushed back up into head.
- This work is reinforcing (in my mind) the need for coming up for a sane naming scheme for "manifests" to avoid ObsoleteFiles.inc bloat in a packaging manifest way, in particular, something like:
CDDL-EXAMPLES.plist
Then for some sections (like MK_EXAMPLES == no) it would glob all of the files that ended in EXAMPLES and load on demand :)... this might be a good first step at having a legitimate packaging plist for things.
That being said, if thing do turn into ports or packaging plists, and pkgng can delete things, then the policy bits can be simplified down to pkgng calls to remove the base system packages. Two bird with one stone!
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 14–39 | This is tricky. I agree that this needs to be handled in a more consistent manner, but the usr/tests part is buggy if someone changes TESTSBASE. Could you please omit deleting these? | |
| 156 | This is missing lib/libauditd and lib/libbsm; see ^/projects/building-blocks for more details: | |
| 159 | Good catch. I forgot to handle this on my branch. | |
| 240–243 | LGTM. Please commit MK_BLUETOOTH. | |
| 292 | Didn't realize this was happening from etc/ until now. Nice catch! | |
| 425 | LGTM (and interesting that my script didn't catch the symlink -- thanks for the testcase :)..) | |
| 480 | MK_BSNMP looks ok. | |
| 944 | Should this be moved down, or committed as a separate change? | |
| 983 | This... is potentially dangerous if MK_CLANG_IS_CC == no and MK_GCC == yes before this. | |
Please don't remove any OLD_DIRS directives until the bits you have have been pushed back up into head
Seems there's a typo so I didn't quite get this. Should I not commit OLD_DIRS removals for now?
Also, could you please be a bit more explicit on what may be committed? For now I'm only sure for BLUETOOTH, BSD_CPIO and BSNMP bits without OLD_DIRS removals.
Also, since these will be basically by first commits to src, could you elaborate on a process a bit? I assume I should commit one chunk (e.g. for single MK_ knob) at a time, with at least following headers:
Differential Revision: https://reviews.freebsd.org/D1600
Approved by: ngie
Are MFC: or other header needed? Should I update differential revision after commits to make it shorter, e.g. won't it lose inline comments?
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 14–39 | Won't just substituting TESTSBASE solve the thing? | |
| 156 | As I've mentioned before, my test compares filetrees produced by KNOB make installworld and make installworld && KNOB make delete-old, and it doesn't show any leftovers. Or is this specific to the branch? | |
| 425 | Are both BOOT and CPIO good? | |
| 944 | sort(1) produces +, -, . order with either C, ru_RU.UTF-8, en_US.UTF-8 locales. May commit as a separate change. | |
| 983 | Why? | |
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 156 | Yes, I suppose it's specific to the branch. That being said, there isn't anything that links [in base] to these libraries. Feel free to omit them if needed and I can loop rwatson in for those on another review once this diff's been committed. | |
| 207 | Please commit this hunk (MK_BINUTILS) | |
| 425 | Yup! | |
| 1681–1686 | Please commit this hunk | |
| 1711 | Let's hold off on committing this because there's some overlap between this and the functional components in here (MK_IPFILTER. MK_INET6, MK_FORTH, etc). | |
| 2142 | Please commit this hunk | |
| 2206 | Please commit this hunk | |
| 2830 | Please commit this hunk (MK_HTML) | |
| 3775–3781 | Uh... let's do find ${DESTDIR} -name \*.a -and \! -name \*_p.a instead for MK_INSTALLLIB... | |
| 5243 | OLD_FILES+=find ${DESTDIR}/usr/lib32 \! -type d, etc | |
| 5898 | Please commit this hunk | |
| 6124 | I'm not so sure about this compile-time option. Please leave this alone.. | |
| 6132 | I'm not so sure about this compile-time option... Please leave this alone. | |
| 6325 | This hunk looks ok to commit | |
| 7406 | This hunk looks ok to commit, but the manpages need to be removed as well, per https://svnweb.freebsd.org/changeset/base/277939 (not your fault.. MLINKS was in the wrong spot). | |
| 7416–7419 | This hunk looks ok to commit | |
| 7444 | Hmmm.. no manpages are being removed here. | |
| 7449 | Shouldn't this line be in MK_MAILWRAPPER == no? | |
| 7454 | This hunk looks good -- please commit. | |
| 7819–7822 | This hunk looks good. Please commit | |
| 7824 | This hunk looks good. Please commit | |
| 7863 | MK_NETGRAPH == no looks ok. Please commit hunk | |
| 7900 | Hunk looks ok for commit | |
| 7965 | MK_NLS looks ok for commit | |
| 8077 | Hunk looks ok for commit (kind of amusing why only tcsh uses this feature...?) | |
| 8100 | Looks ok for commit | |
| 8186 | Looks ok for one-line commit | |
| 8198 | This hunk looks ok for commit | |
| 9712–9716 | This hunk looks ok for commit | |
| 9748 | This hunk looks ok for commit | |
| 9840 | This looks ok for commit | |
| 9853 | find ${DESTDIR}/usr/lib ${DESTDIR}/usr/lib32 -name \*_p.a... | |
| 10138 | Hunk LGTM | |
| 10211 | find ${DESTDIR}/rescue ... | |
| 10606 | This is inconsistent.. | |
| 10610 | find ${DESTDIR}/usr/share/doc | |
| 10695 | Hunk looks ok. Please commit | |
| 10730 | This hunk's ok for commit | |
| 10743 | This hunk's missing svn.1.gz | |
| 10755 | Hunk looks good. Please commit | |
| 10948 | Hunk looks good. Please commit | |
| 11492 | Hunk looks good -- please commit | |
| 11626 | Hunk looks good, please commit. | |
| 11635 | There might be some overlap here. Let's hold off on this diff | |
| 12023 | Hunk looks good -- please commit | |
| 12048 | Hunk looks good -- please commit | |
Yes please. Until the work you've done making OLD_DIRS obsolete is in effect, it's best not to remove OLD_DIRS at all.
Also, could you please be a bit more explicit on what may be committed? For now I'm only sure for BLUETOOTH, BSD_CPIO and BSNMP bits without OLD_DIRS removals.
I've tried to make comments at the beginning of relevant hunks of code.
I've read through the bulk majority of the review, but once you update the diff it should whittle away what's remaining quite a bit more (the problem is that Phabricator shows deltas between diffs by default, but that can be changed). This is part of the reason why I've been using vimdiff between my git checkout of my svn projects branch and an svn checkout of head so extensively -- it makes it easier to produce smaller diffs for review.
Also, since these will be basically by first commits to src, could you elaborate on a process a bit? I assume I should commit one chunk (e.g. for single MK_ knob) at a time, with at least following headers:
Differential Revision: https://reviews.freebsd.org/D1600
Approved by: ngieAre MFC: or other header needed? Should I update differential revision after commits to make it shorter, e.g. won't it lose inline comments
Remove more files when MK_<foo> == no [and MK_<>bar == no, etc]
MFC after: 1 week
Reviewed by: ngie
Differential Revision: D1600
Almost forgot...
Approved by: ngie
is required as well. Sometimes people omit Reviewed by in favor of Approved by, but I leave it in for personal clarity. I don't think there's a hard and fast rule for that, per the Committer's Guide.
Differential Revision: D1600
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 944 | I was using find -s, not find | sort. Not sure what dim@ was using... | |
Bunch of hunks are still not reviewed. I'd like to commit at least some of them before resubmitting the patch:
CALENDAR
CASPER
CUSE
FORTH
INET6
JAIL
KDUMP
KERBEROS
KERBEROS_SUPPORT
LDNS_UTILS
LEGACY_CONSOLE
TESTS_SUPPORT
UTMPX
VI
VT
ZONEINFO
Also, how should I handle MFCs?
One more thing: some options imply deleting configuration files, which is IMO unsafe as user changes may be lost in the process, and configs may still be used (from ports software - for examples, openssh-portable). What do you think of introducing OLD_CONFIGS and corresponding delete-old-configs target which is not by default called by delete-old?
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 944 | I think this is not important, I'll drop this change. | |
| 7444 | The test shows that manpages are installed unconditionally. MAIL-releated staff is generally quite messy though. | |
| 10606 | Still correct according to file list comparison. | |
| 11635 | Just for the node, I've tried to fix other MK_* = no which MK_TOOLCHAIN enforce first, then retest TOOLCHAIN, so it should not contain anything handled by other options. I may have missed something though. | |
LGTM
Also, how should I handle MFCs?
It isn't stated explicitly in the committer's guide how one goes about MFCing changes, but basically what you do is (referencing my mfc script checked into github -- https://github.com/yaneurabeya/scratch/blob/0b8ba9d017a97b4cdfe4197ba021211319d1c622/common/home/ngie/bin/mfc , https://github.com/yaneurabeya/scratch/blob/0b8ba9d017a97b4cdfe4197ba021211319d1c622/common/home/ngie/bin/mfc_log ):
cd /copy/of/stable/[9,10]
mfc ^/head <revision1> [revision2...]
- Resolve conflicts, if any
- Verify your changes with yes | sudo make delete-old both with the knob unset (before) and the knob set (after). If you have a VM instance, snapshots are extremely helpful. If not, ZFS snapshots and rolling back the snapshots is advised.
svn ci -F commit
Be careful when MFCing changes as not all of the build knobs exist on stable/9 and stable/10 (MK_VI for instance).
One more thing: some options imply deleting configuration files, which is IMO unsafe as user changes may be lost in the process, and configs may still be used (from ports software - for examples, openssh-portable). What do you think of introducing OLD_CONFIGS and corresponding delete-old-configs target which is not by default called by delete-old?
If they're using interactive make delete-old, it won't be as much of an issue, but a heads up would probably be advised for any configuration changes (even if it's in UPDATING). mergemaster prompts users if
I don't understand why ports should be using configuration files from base -- that seems broken; in the case of openssh-portable, that doesn't seem to be the case at $work.
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 573 | Please hold off on these changes until your OLD_DIRS removal change has been committed. | |
| 677 | Hunk LGTM -- please commit. | |
| 983 | You'll break cc, etc, which isn't hard to "unbreak", but is a bit painful for most users. | |
| 1077 | Hunk LGTM -- please commit. | |
| 3741 | Hunk LGTM -- please commit. | |
| 4195 | Hunk LGTM -- please commit. | |
| 4201 | Hunk LGTM -- please commit. | |
| 5189 | Hunk LGTM -- please commit. | |
| 5221 | Hunk LGTM -- please commit | |
| 5225 | Hunk LGTM -- please commit | |
| 11551 | Hunk LGTM -- please commit. | |
| 12291 | Please be careful when MFCing this change. This will break rcorder, unless r278706 has been MFCed first. | |
| 12528 | The addition of MK_ZONEINFO LGTM, but are you removing the MK_SVN entries? | |
| tools/build/mk/OptionalObsoleteFiles.inc | ||
|---|---|---|
| 12528 | Those went up to keep the list sorted. I'll probably process the sorting as a separate review. | |
At this stage, it might be a good idea to close this review and open a new one. Sound good?