Page MenuHomeFreeBSD

Remove the OpenBSD derived bc and dc programs (not built by default)
Needs ReviewPublic

Authored by se on Oct 2 2024, 1:48 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 2, 7:39 AM
Unknown Object (File)
Fri, Nov 1, 10:35 AM
Unknown Object (File)
Thu, Oct 31, 6:30 AM
Unknown Object (File)
Wed, Oct 30, 10:52 PM
Unknown Object (File)
Sun, Oct 27, 3:15 PM
Unknown Object (File)
Fri, Oct 25, 4:54 AM
Unknown Object (File)
Oct 18 2024, 12:35 AM
Unknown Object (File)
Oct 17 2024, 11:15 PM

Details

Reviewers
des
imp
Summary

Gavin Howard's bc and dc implementation has been the default version on FreeBSD-CURRENT since 2020-06-27.
The previous version has been kept and could be built using WITHOUT_GH_BC on FreeBSD-13 and -14 (FreeBSD-12 used the previous code unless built with WITH_GH_BC).

Test Plan

Apply patch and build and install the world.
No functional changes are expected, since only code already ignored in buildworld has been removed.
The src.conf.5 man-page should no longer mention WITHOUT_GH_BC.
Building with WITHOUT_GH_BC should fail, since this option is no longer supported.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

se requested review of this revision.Oct 2 2024, 1:48 PM

Is there value in keeping the tests that were added to the old version?

des requested changes to this revision.Oct 2 2024, 5:11 PM

You also need to remove tools/build/options/WITH_GH_BC, move the relevant section of tools/build/mk/OptionalObsoleteFiles.inc to ObsoleteFiles.inc, and regenerate share/man/man5/src.conf.5.

This revision now requires changes to proceed.Oct 2 2024, 5:11 PM

Is there value in keeping the tests that were added to the old version?

Perhaps. More importantly, Gavin Howard's bc has its own test suite which we could adopt. Take a look at how Apple did it: https://github.com/apple-oss-distributions/bc

Update diff to include removal of dependency on MK_GH_BC from usr.bin/Makefile and from tools/build/mk/OptionalObsoleteFiles.inc.

In D46876#1068928, @des wrote:

Is there value in keeping the tests that were added to the old version?

Perhaps. More importantly, Gavin Howard's bc has its own test suite which we could adopt. Take a look at how Apple did it: https://github.com/apple-oss-distributions/bc

These tests have been there for more than 4 years, see the contents of /usr/tests/usr.bin/gh-bc ...

In D46876#1068925, @des wrote:

You also need to remove tools/build/options/WITH_GH_BC, move the relevant section of tools/build/mk/OptionalObsoleteFiles.inc to ObsoleteFiles.inc, and regenerate share/man/man5/src.conf.5.

I have removed the option from the options directory. A prior version of the patch included the updated share/man/man5/src.conf.5, but phabricator tagged it with "does not need to be reviewed" or something to that effect. I'll update the patch to include the generated src.conf.5. That update will also include the change to ObsoleteFiles.inc which I forgot to include in the diff generated for the review ...

phabricator tagged it with "does not need to be reviewed"

Phabricator recognizes it as a generated file and so doesn't need to be reviewed. The note is intended more for the assigned reviewers - the change can be included, Phab just notes that reviewers don't need to look at it.

Also - if you have an update to make for this review, please generate diffs via git diff -U999999 or git show -U999999 so that the rest of the file context is available. (In fact I'm a little curious about the process you used to update this review, since share/mk/src.opts.mk has context but other files do not).

Address review comments.

phabricator tagged it with "does not need to be reviewed"

Phabricator recognizes it as a generated file and so doesn't need to be reviewed. The note is intended more for the assigned reviewers - the change can be included, Phab just notes that reviewers don't need to look at it.

I see ...

Also - if you have an update to make for this review, please generate diffs via git diff -U999999 or git show -U999999 so that the rest of the file context is available. (In fact I'm a little curious about the process you used to update this review, since share/mk/src.opts.mk has context but other files do not).

My sources contain a number of uncommitted changes, I did not know how to create the diff in one go (after removal of the usr.bin/bc and usr.bin/dc directories), there might be a git option that allows to consider missing files as deleted, but I'll never get used to the abonimation named git and did not know about a better method than incrementally adding to the initial diff (were it did not forget the -U99999 that was missing in a few of the other diffs).

Stefan, I highly recommend that you install freebsd-git-devtools and use git arc to prepare and stage reviews:

  • Commit your changes to a local branch.
  • Run git arc create -r des,imp <ref> to create a Phabricator review of the latest commit with myself and Warner as reviewers (<ref> is a reference to the commit; it can be a hash, HEAD for the latest commit on the current branch, a branch name for the latest commit on another branch, etc.)
  • Once the review is approved, switch to main and run git arc stage <ref> to copy the commit to the current branch and edit the commit message to add “Reviewed by” and “Differential” lines. Note that <ref> here must be a reference to the original commit (or a copy of it) on another branch. Also note that git arc stage will insert a blank line in the commit message which you may have to remove.
  • Double-check and push. If you're not satisfied with what git arc stage did, you can always amend (git commit --amend) before pushing or even revert (git reset --hard freebsd/main) and start over. The original commit will be left untouched on the branch you created it on.