Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 356389 - dodoc ignores exit status of underlying install
Summary: dodoc ignores exit status of underlying install
Status: CONFIRMED
Alias: None
Product: Portage Development
Classification: Unclassified
Component: Core - Ebuild Support (show other bugs)
Hardware: All Linux
: High normal (vote)
Assignee: Portage team
URL:
Whiteboard:
Keywords:
: 409755 (view as bug list)
Depends on: 351697 365809 365859 365861 365865 366011 367377 367489 367499 367505 367547 367609 367665 367667 367677 367681 367689 367703 367885 367887 368883 368893 369009 369111 369763 369961 370095 371023 371821 374811 375619 376205 377235 387815 409793
Blocks:
  Show dependency tree
 
Reported: 2011-02-25 07:36 UTC by Ulrich Müller
Modified: 2017-08-11 21:05 UTC (History)
3 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
proposed patch (0001-dodoc-Honour-exit-status-of-install-bug-356389.patch,1.20 KB, patch)
2011-02-25 07:38 UTC, Ulrich Müller
Details | Diff
updated patch (0001-dodoc-Honour-exit-status-of-install-bug-356389.patch,1.52 KB, patch)
2011-02-25 08:55 UTC, Ulrich Müller
Details | Diff
updated patch (0001-dodoc-Honour-exit-status-of-install-bug-356389.patch,1.50 KB, patch)
2011-02-25 08:59 UTC, Ulrich Müller
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ulrich Müller gentoo-dev 2011-02-25 07:36:14 UTC
dodoc calls 'install -m0644 "${x}" "${dir}"' but ignores its exit status. Especially, if a directory is given as argument (and without -r), no error will be signalled. Worse, it will be queued up for ecompress and makes it fail.

PMS is quite explicit on this point: "it is an error for a directory to be specified."

Patch will follow.
Comment 1 Ulrich Müller gentoo-dev 2011-02-25 07:38:23 UTC
Created attachment 263759 [details, diff]
proposed patch

This fixes the problem for me.
Comment 2 Ulrich Müller gentoo-dev 2011-02-25 07:43:01 UTC
I forgot to mention, the install command will already output a useful error message (like "install: omitting directory `foo/bar'"). Therefore I haven't added another message in dodoc.
Comment 3 Zac Medico gentoo-dev 2011-02-25 08:25:59 UTC
We need to be careful, especially for earlier EAPIs, that we're not making ebuilds die due to passing directories as arguments (possibly matched via dodoc * or soemthing like that). We could use eqawarn instead of an error for this case.
Comment 4 Ulrich Müller gentoo-dev 2011-02-25 08:38:36 UTC
*sigh* But probably you're right. Could it be made an error in EAPI 4 at least?

And maybe the QA warning in earlier EAPIs could be changed to an error after some time?
Comment 5 Zac Medico gentoo-dev 2011-02-25 08:40:53 UTC
Sure, that sounds fine, as long as we release it soon (before any EAPI 4 ebuilds are marked stable).
Comment 6 Ulrich Müller gentoo-dev 2011-02-25 08:55:15 UTC
Created attachment 263763 [details, diff]
updated patch
Comment 7 Ulrich Müller gentoo-dev 2011-02-25 08:59:23 UTC
Created attachment 263765 [details, diff]
updated patch
Comment 8 Zac Medico gentoo-dev 2011-02-25 18:20:10 UTC
(In reply to comment #7)
> Created an attachment (id=263765) [details]
> updated patch

I went ahead and committed your patch here:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=26ba46a9e620c5dd5d3699a854a68ab8cab04464
Comment 9 Zac Medico gentoo-dev 2011-02-25 20:00:22 UTC
For bug #356461, I've made EAPI 4 dodoc a symlink to doins:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=23694b1dd9c6400da68ea2029e9f190994a631ac

The doins error handling be equivalent to the intended dodoc error handling.
Comment 10 Zac Medico gentoo-dev 2011-02-25 20:35:35 UTC
(In reply to comment #9)
> The doins error handling be equivalent to the intended dodoc error handling.

Fixed now:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=15e251fdae81c43e78443587f0eb19b0ca158bd3
Comment 11 Zac Medico gentoo-dev 2011-03-27 23:18:41 UTC
This is fixed in 2.1.9.42.
Comment 12 Brian Harring (RETIRED) gentoo-dev 2011-05-03 12:46:59 UTC
This however breaks EAPI compatibility for <EAPI4, with 'dodoc some-directory' via no longer returning a nonzero exit, evidenced by bug #365809 (eapi-3 instance, doing exactly what I described).
Comment 13 Zac Medico gentoo-dev 2011-05-03 13:56:35 UTC
(In reply to comment #12)
> This however breaks EAPI compatibility for <EAPI4, with 'dodoc some-directory'
> via no longer returning a nonzero exit, evidenced by bug #365809 (eapi-3
> instance, doing exactly what I described).

Here you can see that the old behavior was to ignore the exit status of install:

http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=26ba46a9e620c5dd5d3699a854a68ab8cab04464

We're between a rock and a hard place when the spec breaks backward compatibility.
Comment 14 Brian Harring (RETIRED) gentoo-dev 2011-05-03 14:33:26 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > This however breaks EAPI compatibility for <EAPI4, with 'dodoc some-directory'
> > via no longer returning a nonzero exit, evidenced by bug #365809 (eapi-3
> > instance, doing exactly what I described).
> 
> Here you can see that the old behavior was to ignore the exit status of
> install:
> 
> http://git.overlays.gentoo.org/gitweb/?p=proj/portage.git;a=commit;h=26ba46a9e620c5dd5d3699a854a68ab8cab04464
> 
> We're between a rock and a hard place when the spec breaks backward
> compatibility.

Not really; the spec's terms there are basically true; portage however was inconsistent in it's returning.  Trace the -e clause- note it was reporting there.

As for breaking backwards compatibility... this is arguable; tracing it back to '05, the return code bled through- so it would show, although not for missing files.  It wasn't until 5169f55601c29cb5e72361b3254b71a41af2d5b1 the ret code showed up, which was implemented incorrectly inverting the situation.

Either way... this one *is* fixable in portage.  Consider the error case; current portage ignores the error, despite ebuilds trying to catch that error via || die's.  The tree's literally litered with that- meaning that was their intent all along, just portage was misbehaving.
Comment 15 Zac Medico gentoo-dev 2011-05-03 15:56:32 UTC
Here's a copy from 2003:

http://sources.gentoo.org/cgi-bin/viewvc.cgi/gentoo-src/portage/bin/dodoc?revision=1.5

The returncode will bleed through for the last dodoc argument, assuming that gzip will fail if install fails. So, a directory will only trigger failure here if it's the last argument, which is quite unpredictable.
Comment 16 Ciaran McCreesh 2011-05-03 16:13:51 UTC
I'd say that it's pretty clear on this one that people expect dodoc to "do the right thing", and that Portage's behaviour is sufficiently weird and unspecifiable that we can get away with just having Portage do what PMS says and claim that earlier behaviour was a bug.
Comment 17 Zac Medico gentoo-dev 2011-05-03 16:20:05 UTC
(In reply to comment #16)
> I'd say that it's pretty clear on this one that people expect dodoc to "do the
> right thing", and that Portage's behaviour is sufficiently weird and
> unspecifiable that we can get away with just having Portage do what PMS says
> and claim that earlier behaviour was a bug.

Well, if we choose to make the error handling stricter (backward incompatible) as specified by PMS, then we potentially expose ourselves to an unknown of bug reports similar to bug 365809. I think that portage's QA notice is a good compromise since it reports the error but does not inconvenience users by triggering more reports like bug 365809.
Comment 18 Ciaran McCreesh 2011-05-03 16:33:44 UTC
We'd not be exposing users to breakage. We'd be ensuring that users get an error where something is broken, as opposed to the current situation where Portage fails to detect that an error has occurred.
Comment 19 Zac Medico gentoo-dev 2011-05-03 18:11:20 UTC
(In reply to comment #18)
> We'd not be exposing users to breakage. We'd be ensuring that users get an
> error where something is broken, as opposed to the current situation where
> Portage fails to detect that an error has occurred.

Nobody knows how many ebuilds that currently build successfully will fail to build after making dodoc more strict. Usually you PMS folks champion backward-compatibility, so it seems odd that now you're pushing for a backward-incompatible change. I don't feel comfortable with this change, so I would like to submit it to the council.
Comment 20 Ulrich Müller gentoo-dev 2011-05-03 19:39:18 UTC
Removing pms-bugs from CC. I don't see what would be wrong with the spec here.

I suggest that we leave portage behaviour as it is, at least for some time. A QA warning for EAPIs less than 4 will avoid unnecessary breakage for users, and give maintainers some time to correct their ebuilds. But after some time (one year?) the warning should be changed to an error.
Comment 21 Petteri Räty (RETIRED) gentoo-dev 2011-05-04 10:41:48 UTC
(In reply to comment #19)
> 
> Nobody knows how many ebuilds that currently build successfully will fail to
> build after making dodoc more strict. Usually you PMS folks champion
> backward-compatibility, so it seems odd that now you're pushing for a
> backward-incompatible change. I don't feel comfortable with this change, so I
> would like to submit it to the council.

We are collecting agenda items for the next council meeting so feel free to suggest it. I would though expose this to public mailing list discussion before that.
Comment 22 Zac Medico gentoo-dev 2011-05-04 16:00:20 UTC
(In reply to comment #21)
> (In reply to comment #19)
> > 
> > Nobody knows how many ebuilds that currently build successfully will fail to
> > build after making dodoc more strict. Usually you PMS folks champion
> > backward-compatibility, so it seems odd that now you're pushing for a
> > backward-incompatible change. I don't feel comfortable with this change, so I
> > would like to submit it to the council.
> 
> We are collecting agenda items for the next council meeting so feel free to
> suggest it. I would though expose this to public mailing list discussion before
> that.

Thanks, but if all parties will settle for handling it eqawarn for now, then the council doesn't need to get involved. As suggested in comment #20, the current spec is workable as long as we take appropriate precautions when implementing it.
Comment 23 Zac Medico gentoo-dev 2012-03-26 16:14:13 UTC
*** Bug 409755 has been marked as a duplicate of this bug. ***
Comment 24 Zac Medico gentoo-dev 2017-08-11 21:05:41 UTC
It's still just a warning for EAPI 3 and earlier, but those EAPIs are increasingly irrelevant.

It's fatal in EAPI 4 and later (EAPIs that support dodoc -r):

https://gitweb.gentoo.org/proj/portage.git/commit/?id=15e251fdae81c43e78443587f0eb19b0ca158bd3