[gentoo-portage-dev] [PATCH v3] install-qa-check: New QA check/cleanup for empty directories

2018-01-29 Thread Michał Górny
Warn about empty directories installed to /var in install-qa-check phase
(that were not "filled" using keepdir), to help developers stop relying
upon Portage preserving them. Those directories are rather unlikely to
be false positives.

Furthermore, remove all the empty directories if FEATURES=strict-keepdir
is used to catch even more problems (intended for developers). Here
warnings are not really suitable since there will be a high number
of false positives.

The PMS specifies the behavior upon merging empty directories
as undefined, and specifically prohibits ebuilds from attempting
to install empty directories. However, ebuilds occasionally still fall
into the trap of relying on 'dodir' preserving the directory. Make
the Portage behavior more strict in order to prevent that.
---
 bin/install-qa-check.d/95empty-dirs | 42 +
 man/make.conf.5 |  4 
 pym/portage/const.py|  1 +
 3 files changed, 47 insertions(+)
 create mode 100644 bin/install-qa-check.d/95empty-dirs

diff --git a/bin/install-qa-check.d/95empty-dirs 
b/bin/install-qa-check.d/95empty-dirs
new file mode 100644
index 0..0d06b278d
--- /dev/null
+++ b/bin/install-qa-check.d/95empty-dirs
@@ -0,0 +1,42 @@
+# Warn about and/or remove empty directories installed by ebuild.
+
+# Rationale: PMS prohibits ebuilds from installing empty directories.
+# Cleaning them up from the installation image provides an easy way
+# to make sure that ebuilds are not relying on it while making it easy
+# for users to override this if they need to.
+#
+# The ebuilds that need to preserve empty directories should use keepdir
+# as documented e.g.:
+# https://devmanual.gentoo.org/function-reference/install-functions/index.html
+#
+# For now, we emit QA warnings for empty directories in /var.
+# Additionally, if FEATURES=strict-keepdir is enabled we explicitly
+# remove *all* empty directories to trigger breakage.
+
+find_empty_dirs() {
+   local warn_dirs=()
+   local d striparg=
+
+   [[ ${FEATURES} == *strict-keepdir* ]] && striparg=-delete
+
+   while IFS= read -r -d $'\0' d; do
+   [[ ${d} == ${ED%/}/var/* ]] && warn_dirs+=( "${d}" )
+   done < <(find "${ED}" -depth -mindepth 1 -type d -empty -print0 
${striparg} | sort -z)
+
+   if [[ ${warn_dirs[@]} ]]; then
+   eqawarn "One or more empty directories installed to /var:"
+   eqawarn
+   for d in "${warn_dirs[@]}"; do
+   eqawarn "  ${d#${ED%/}}"
+   done
+   eqawarn
+   eqawarn "If those directories need to be preserved, please make 
sure to create"
+   eqawarn "or mark them for keeping using 'keepdir'. Future 
versions of Portage"
+   eqawarn "will strip empty directories from installation image."
+   fi
+}
+
+find_empty_dirs
+: # guarantee successful exit
+
+# vim:ft=sh
diff --git a/man/make.conf.5 b/man/make.conf.5
index a81b497bd..cb0f00237 100644
--- a/man/make.conf.5
+++ b/man/make.conf.5
@@ -623,6 +623,10 @@ see \fBinstallsources\fR.
 Have portage react strongly to conditions that have the potential to be
 dangerous (like missing or incorrect digests for ebuilds).
 .TP
+.B strict-keepdir
+Have portage strictly require keepdir calls in ebuilds.  Empty
+directories installed without explicit keepdir will be removed.
+.TP
 .B stricter
 Have portage react strongly to conditions that may conflict with system
 security provisions (for example textrels, executable stack).  Read about
diff --git a/pym/portage/const.py b/pym/portage/const.py
index e5fa4b67c..655be82b1 100644
--- a/pym/portage/const.py
+++ b/pym/portage/const.py
@@ -184,6 +184,7 @@ SUPPORTED_FEATURES   = frozenset([
"split-elog",
"split-log",
"strict",
+   "strict-keepdir",
"stricter",
"suidctl",
"test",
-- 
2.16.1




Re: [gentoo-portage-dev] Re: [PATCH] emerge: add --changed-deps-report option (bug 645780)

2018-01-29 Thread Zac Medico
On 01/29/2018 07:29 PM, Duncan wrote:
> Zac Medico posted on Sun, 28 Jan 2018 22:21:48 -0800 as excerpted:
> 
>> On 01/28/2018 09:49 PM, Zac Medico wrote:
 3) Show a NOTE telling users about --changed-deps=y
>>>
>>> This is in the HINT section, which is displayed if both --changed-deps
>>> and --dynamic-deps are disabled in PATCH v2.
>>
>> Actually, the whole report should be suppressed if either --changed-deps
>> or --dynamic-deps is enabled, so I'll send PATCH v4 for that.
> 
> This is shaping up quite nicely and by (1) dramatically shortening the 
> original "wall of text" report and (2) aborting the report if no affected 
> packages are in the graph, it's vastly improved from the original.
> 
> I definitely expect it to be rather helpful here, since I have both 
> --dynamic-deps and --changed-deps off by default, and seeing that list 
> could be /quite/ helpful.  Looking forward to it! =:^)

Great!

> My remaining concern, and I'm not sure there's a solution, is that for 
> routine 30-day-plus updaters, the warning could quickly become "routine 
> noise", due to valid no-r-bump exceptions such as the llvm example mgorny 
> provided, which very well /could/ happen often enough to trigger the 
> warning nearly every time for 30-day-plus updaters.  Then when it really 
> counts and could help, people will likely be ignoring it.

Until we invent something better, people will have to set
EMERGE_DEFAULT_OPTS="--changed-deps-report=n" if it bothers them too
much. This is acceptable to me because my main goal is simply to make
people aware of --changed-deps when they need it most. If the set
--changed-deps-report=n then it's their responsibility to know when to
use --changed-deps.

> Maybe someone else has an idea, but as I said it's already vastly 
> improved from the original, and I believe usable as-is, now, while I'd 
> have found the original quite irritating by about the third time I saw 
> it, even if also helpful.

Great, thanks for the feedback!
-- 
Thanks,
Zac



signature.asc
Description: OpenPGP digital signature


[gentoo-portage-dev] Re: [PATCH] emerge: add --changed-deps-report option (bug 645780)

2018-01-29 Thread Duncan
Zac Medico posted on Sun, 28 Jan 2018 22:21:48 -0800 as excerpted:

> On 01/28/2018 09:49 PM, Zac Medico wrote:
>>> 3) Show a NOTE telling users about --changed-deps=y
>> 
>> This is in the HINT section, which is displayed if both --changed-deps
>> and --dynamic-deps are disabled in PATCH v2.
> 
> Actually, the whole report should be suppressed if either --changed-deps
> or --dynamic-deps is enabled, so I'll send PATCH v4 for that.

This is shaping up quite nicely and by (1) dramatically shortening the 
original "wall of text" report and (2) aborting the report if no affected 
packages are in the graph, it's vastly improved from the original.

I definitely expect it to be rather helpful here, since I have both 
--dynamic-deps and --changed-deps off by default, and seeing that list 
could be /quite/ helpful.  Looking forward to it! =:^)

My remaining concern, and I'm not sure there's a solution, is that for 
routine 30-day-plus updaters, the warning could quickly become "routine 
noise", due to valid no-r-bump exceptions such as the llvm example mgorny 
provided, which very well /could/ happen often enough to trigger the 
warning nearly every time for 30-day-plus updaters.  Then when it really 
counts and could help, people will likely be ignoring it.

Maybe someone else has an idea, but as I said it's already vastly 
improved from the original, and I believe usable as-is, now, while I'd 
have found the original quite irritating by about the third time I saw 
it, even if also helpful.

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman




Re: [gentoo-portage-dev] Re: [PATCH v2 3/3] _emerge.Ebuild*: delay creating DISTDIR shadow until src_unpack

2018-01-29 Thread Michał Górny
Dnia 29 stycznia 2018 09:07:47 CET, Michael Haubenwallner  
napisał(a):
>On 01/25/2018 10:11 AM, Michał Górny wrote:
>> W dniu czw, 25.01.2018 o godzinie 10∶07 +0100, użytkownik Michael
>> Haubenwallner napisał:
>>> Hi,
>>>
>>> ${Subject} ringing a bell here:
>>>
>>> dev-db/oracle-instantclient is fetch restricted. As a binary package
>with
>>> multiple USE options there's a bunch of files to download - even for
>>> multiple archs when multilib is active.
>>>
>>> So in pkg_nofetch() I'm telling the user whether a file to download
>is
>>> "already here" or "still absent", by testing if $A exists in
>$DISTDIR.
>>>
>>> With ${Subject}, I'm wondering if DISTDIR is created for pkg_nofetch
>too.
>>>
>>
>> You're doing the wrong thing then. DISTDIR is not allowed
>> in pkg_nofetch().
>
>Is there a supported way to tell the user exactly which files are still
>missing?

Sounds like a featurereq for Portage itself.

>
>> Furthermore, you're touching files whose hashes have
>> not been verified which is twice wrong.
>
>Well - does portage actually provide unverified files in the shadow
>DISTDIR?

No. The phases in which the directory is present are only run if Manifest 
verification succeeds.

>
>Thanks!
>/haubi/


-- 
Best regards,
Michał Górny (by phone)



[gentoo-portage-dev] Re: [PATCH v2 3/3] _emerge.Ebuild*: delay creating DISTDIR shadow until src_unpack

2018-01-29 Thread Michael Haubenwallner
On 01/25/2018 10:11 AM, Michał Górny wrote:
> W dniu czw, 25.01.2018 o godzinie 10∶07 +0100, użytkownik Michael
> Haubenwallner napisał:
>> Hi,
>>
>> ${Subject} ringing a bell here:
>>
>> dev-db/oracle-instantclient is fetch restricted. As a binary package with
>> multiple USE options there's a bunch of files to download - even for
>> multiple archs when multilib is active.
>>
>> So in pkg_nofetch() I'm telling the user whether a file to download is
>> "already here" or "still absent", by testing if $A exists in $DISTDIR.
>>
>> With ${Subject}, I'm wondering if DISTDIR is created for pkg_nofetch too.
>>
>
> You're doing the wrong thing then. DISTDIR is not allowed
> in pkg_nofetch().

Is there a supported way to tell the user exactly which files are still missing?

> Furthermore, you're touching files whose hashes have
> not been verified which is twice wrong.

Well - does portage actually provide unverified files in the shadow DISTDIR?

Thanks!
/haubi/