Bug 190660 - freebsd-update(8) erroneous message when "fetch install" used
Summary: freebsd-update(8) erroneous message when "fetch install" used
Status: Closed FIXED
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 10.0-RELEASE
Hardware: Any Any
: --- Affects Some People
Assignee: Guangyuan Yang
URL:
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-06-05 13:49 UTC by mail
Modified: 2018-09-10 10:18 UTC (History)
7 users (show)

See Also:


Attachments
Remove erroneous message when "fetch install" used (330 bytes, patch)
2016-02-08 07:28 UTC, mail
no flags Details | Diff
patch to revert return code back to 1 (453 bytes, patch)
2018-06-29 20:23 UTC, Kenneth Salerno
no flags Details | Diff
patch to return 1 if install is run without fetch and nothing to install (407 bytes, patch)
2018-06-29 23:50 UTC, Kenneth Salerno
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mail 2014-06-05 13:49:21 UTC
freebsd-update tell the user to run 'fetch' first even though we're using the fetch command.

# freebsd-update fetch install
Looking up update.FreeBSD.org mirrors... 5 mirrors found.
Fetching metadata signature for 10.0-RELEASE from update4.freebsd.org... done.
Fetching metadata index... done.
Inspecting system... done.
Preparing to download files... done.

No updates needed to update system to 10.0-RELEASE-p5.
No updates are available to install.
Run '/usr/sbin/freebsd-update fetch' first.
Comment 1 jflemer 2014-11-17 14:37:57 UTC
Additionally, the exit code is 1 in this case.  As far as I can tell this is a non-error (simply that no updates are needed), but this makes scripting annoying since the exit code is a failure.  (Same use and error as OP for me.)
Comment 2 mail 2016-02-08 07:28:55 UTC
Created attachment 166733 [details]
Remove erroneous message when "fetch install" used

Exiting with zero status here when "fetch" has no updates seems reasonable, instead of attempting to execute an "install" which will abort due to lack of updates*.

* unless there are already updates pending that were previously fetched by the user. This feels like unexpected behaviour to me, though.

freebsd-update's ability to chain commands seems like a design mistake to me. For example it allows the admin to issue the commands:

freebsd-update install rollback
freebsd-update fetch rollback

Or even:

freebsd-update fetch fetch fetch fetch fetch fetch fetch fetch fetch fetch
Comment 3 mail 2016-04-01 03:01:18 UTC
I read recently that freebsd-update will be obsoleted by pkg(8) for FreeBSD 11, so it seems safe to assume that there's no incentive to fix this or any other problems freebsd-update(8) has. Which is fair enough.
Comment 4 Ed Maste freebsd_committer freebsd_triage 2016-12-20 22:51:29 UTC
Regardless of what happens in FreeBSD 11 and later we still need to support FreeBSD-update for 10.x, and some usability improvements are warranted.
Comment 5 Guangyuan Yang freebsd_committer freebsd_triage 2017-08-15 16:04:37 UTC
In my opinion, the approach of the proposed patch is not clean enough, since the behaviors of compound statements should always be equivalent to running it separately, for example:

freebsd-update fetch install

should have the same effect as 

freebsd-update fetch
freebsd-update install

The real question here is, should `install` return 0 or 1 (currently 1) if there's nothing to install?
Comment 6 Ed Maste freebsd_committer freebsd_triage 2017-08-15 16:24:15 UTC
> The real question here is, should `install` return 0 or 1 (currently 1) if
> there's nothing to install?

In my opinion doing nothing when there's nothing to install is not an error, so should return 0.

It would be nice to omit the "Run '/usr/sbin/freebsd-update fetch' first." message when run as a a single freebsd-update invocation. The "no updates" messages are still accurate and fine, IMO.
Comment 7 Guangyuan Yang freebsd_committer freebsd_triage 2017-08-16 17:12:22 UTC
Submitted a review to fix this bug.

Differential Revision: https://reviews.freebsd.org/D12037
Comment 8 commit-hook freebsd_committer freebsd_triage 2017-10-09 16:35:10 UTC
A commit references this bug:

Author: ygy
Date: Mon Oct  9 16:33:37 UTC 2017
New revision: 324441
URL: https://svnweb.freebsd.org/changeset/base/324441

Log:
  Fix freebsd-update(8) erroneous message and exit status when "fetch install" used.

  PR:		190660
  Reviewed by:	allanjude
  Approved by:	emaste
  Differential Revision:	https://reviews.freebsd.org/D12037

Changes:
  head/usr.sbin/freebsd-update/freebsd-update.sh
Comment 9 commit-hook freebsd_committer freebsd_triage 2018-03-15 10:14:15 UTC
A commit references this bug:

Author: eadler
Date: Thu Mar 15 10:13:25 UTC 2018
New revision: 330985
URL: https://svnweb.freebsd.org/changeset/base/330985

Log:
  MFC r324441:

  Fix freebsd-update(8) erroneous message and exit status when "fetch install" used.

  PR:		190660

Changes:
_U  stable/11/
  stable/11/usr.sbin/freebsd-update/freebsd-update.sh
Comment 10 Kenneth Salerno 2018-06-29 20:23:10 UTC
Created attachment 194742 [details]
patch to revert return code back to 1

Switching the return code to 0 broke my refresh script logic:

freebsd-update fetch install >/dev/null 2>&1 && \
echo "done. Rebooting system in three (3) seconds." && \
sleep 3 && shutdown -r now && exit 1 ||\
echo "no updates to install."

Can we please revert just that one change back to "exit 1"? See attached mini patch.
Comment 11 Guangyuan Yang freebsd_committer freebsd_triage 2018-06-29 20:54:07 UTC
There
Comment 12 Guangyuan Yang freebsd_committer freebsd_triage 2018-06-29 21:09:40 UTC
(Please ignore the last message - my Chromium crashed)

IMO there are 3 possible results of freebsd-update install:

Installed successfully => exit 0;
Nothing to install => exit 0;
  - fetch was run in a chain of commands right before install, exit 0
  - fetch was not run immediately before - additional message displayed, still exit 0
Installed unsuccessfully => exit 1;

We decided to change "Nothing to install" from 1 to 0 since it seems reasonable to think of it as "did nothing successfully". But if you interpret the return code as "is freebsd-update actually installed *something* successfully", then the version before this PR is correct.

I can see that you are using the error code to decide if a restart is needed. I don't think just revert it back is a good idea, since exitting 0 cause there's nothing to install makes sense to me. I would hope there's another way of telling that, or we could do something like this:

Nothing to install => exit 0;
  - fetch was run in a chain of commands right before install, exit 0
  - fetch was not run immediately before - additional message displayed, exit 1
Comment 13 Kenneth Salerno 2018-06-29 22:16:33 UTC
(In reply to Guangyuan Yang from comment #12)
Hi,

There are four possible outcomes to running "install" so I believe we need to differentiate the return codes:

1. Installed successfully: 0
2. Nothing to install (and fetch was run): 2
3. Nothing to install (and fetch was not run): 1
4. Install ran and failed: 1
Comment 14 Allan Jude freebsd_committer freebsd_triage 2018-06-29 22:19:49 UTC
(In reply to Kenneth Salerno from comment #13)
I don't think it makes sense to conflate option 3 and 4 here. 'Nothing to install' is not really an error, and 'install failed' is an important error to catch
Comment 15 Kenneth Salerno 2018-06-29 23:50:21 UTC
Created attachment 194762 [details]
patch to return 1 if install is run without fetch and nothing to install

This patch is a compromise: return 1 if "install" is run without "fetch" and nothing to install, consistent with error message "run fetch first", therefore should return an error code 1 as before. Maintains the return code of 0 (no error) if user uses both parameters "fetch install" and nothing to install.