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.
Created attachment 263759 [details, diff] proposed patch This fixes the problem for me.
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.
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.
*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?
Sure, that sounds fine, as long as we release it soon (before any EAPI 4 ebuilds are marked stable).
Created attachment 263763 [details, diff] updated patch
Created attachment 263765 [details, diff] updated patch
(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
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.
(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
This is fixed in 2.1.9.42.
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).
(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.
(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.
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.
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.
(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.
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.
(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.
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.
(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.
(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.
*** Bug 409755 has been marked as a duplicate of this bug. ***
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
(In reply to Zac Medico from comment #24) > It's fatal in EAPI 4 and later (EAPIs that support dodoc -r): > > https://gitweb.gentoo.org/proj/portage.git/commit/ > ?id=15e251fdae81c43e78443587f0eb19b0ca158bd3 Closing.
