[gentoo-portage-dev] [PATCH 0/1] Test PMS-compliant usage of the ROOT variable.

[gentoo-portage-dev] [PATCH 0/1] Test PMS-compliant usage of the ROOT variable.

[Michael Orlitzky]

Per the discussion over on -dev, this adds a test case for usage of
the ROOT variable outside of the PMS-defined pkg_* functions (which is
not allowed).

There should probably be a warning for this in repoman, since most
usages I've seen are really intended to be EPREFIX and not ROOT. Any
cases that are not so easy to fix will stand out once the trivial ones
are. Perhaps in EAPI-$next, the warning can become an error.

Michael Orlitzky (1):
  Add a test case for PMS-compliant usage of the ROOT variable.

 ebuild-test/root-var-usage/metadata.xml            | 24 ++++++
 ebuild-test/root-var-usage/root-var-usage-0.ebuild | 90 ++++++++++++++++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 ebuild-test/root-var-usage/metadata.xml
 create mode 100644 ebuild-test/root-var-usage/root-var-usage-0.ebuild

-- 
2.7.3

[gentoo-portage-dev] [PATCH 1/1] Add a test case for PMS-compliant usage of the ROOT variable.

[Michael Orlitzky]

A recent thread on gentoo-dev proposed a change to the devmanual's
description of the ROOT variable:

https://archives.gentoo.org/gentoo-dev/message/8901669dd375ca0fdb610efef0ddfe6f

The proposed change would bring the devmanual's language in line with
the PMS. That discussion reveals that the use of ROOT outside of
pkg_* is illegal, yet current versions of repoman/portage allow it.
This commit adds a test ebuild that violates the PMS (with respect to
ROOT) in several ways.
---
 ebuild-test/root-var-usage/metadata.xml            | 24 ++++++
 ebuild-test/root-var-usage/root-var-usage-0.ebuild | 90 ++++++++++++++++++++++
 2 files changed, 114 insertions(+)
 create mode 100644 ebuild-test/root-var-usage/metadata.xml
 create mode 100644 ebuild-test/root-var-usage/root-var-usage-0.ebuild

diff --git a/ebuild-test/root-var-usage/metadata.xml b/ebuild-test/root-var-usage/metadata.xml
new file mode 100644
index 0000000..f8ba4d0
--- /dev/null
+++ b/ebuild-test/root-var-usage/metadata.xml
@@ -0,0 +1,24 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE pkgmetadata SYSTEM "http://www.gentoo.org/dtd/metadata.dtd">
+<pkgmetadata>
+  <longdescription>
+    Test that the $ROOT variable is used according to the Package
+    Manager Specification (PMS).
+
+    All currently-approved versions of the PMS state that the $ROOT
+    variable is legal only in the pkg_* phases. One common misuse is
+    to use $ROOT instead of $EPREFIX as "something more general than a
+    front-slash" in the src_* functions. We want to detect these
+    mistakes, and eventually eliminate all uses of $ROOT outside of
+    the PMS-defined pkg_* phases.
+  </longdescription>
+
+<maintainer type="person">
+	<email>exampledev@gentoo.org</email>
+	<description>Primary maintainer</description>
+</maintainer>
+<maintainer type="project">
+	<email>exampleproject@gentoo.org</email>
+	<name>Gentoo Example Project</name>
+</maintainer>
+</pkgmetadata>
diff --git a/ebuild-test/root-var-usage/root-var-usage-0.ebuild b/ebuild-test/root-var-usage/root-var-usage-0.ebuild
new file mode 100644
index 0000000..a46339e
--- /dev/null
+++ b/ebuild-test/root-var-usage/root-var-usage-0.ebuild
@@ -0,0 +1,90 @@
+# Copyright 1999-2016 Gentoo Foundation
+# Distributed under the terms of the GNU General Public License v2
+# $Id$
+
+EAPI=6
+
+DESCRIPTION="Ensure ROOT variable usage complies with the PMS"
+HOMEPAGE="https://dev.gentoo.org/~ulm/pms/head/pms.html#x1-11800011"
+LICENSE="BSD"
+SLOT="0"
+KEYWORDS="~amd64"
+IUSE=""
+
+# All uses of $ROOT within the pkg_* "Ebuild-defined functions" should
+# be allowed. The list comes from the so-named chapter of the PMS.
+
+pkg_pretend() {
+	local foo=$ROOT
+}
+
+pkg_setup() {
+	local foo="${ROOT}/foo"
+}
+
+pkg_preinst() {
+	local foo="$ROOT"
+}
+
+pkg_postinst() {
+	local foo="bar/${ROOT}"
+}
+
+pkg_prerm() {
+	local foo="bar/${ROOT}/baz"
+}
+
+pkg_postrm() {
+	local foo=bar/$ROOT
+}
+
+pkg_config() {
+		local foo="$ROOT"
+}
+
+pkg_info() {
+local foo=$ROOT
+}
+
+pkg_nofetch() {
+	local foo=`echo $ROOT`
+}
+
+# All uses below here are errors. The following src_* functions are
+# defined in the PMS.
+src_unpack() {
+	local foo=$ROOT
+}
+
+src_prepare() {
+	local foo="${ROOT}/foo"
+}
+
+src_configure() {
+	local foo=`echo $ROOT`
+}
+
+src_compile() {
+local foo=$ROOT
+}
+
+src_test() {
+		local foo=$(echo $ROOT)
+}
+
+src_install() {
+	local foo="bar/${ROOT}/baz"
+}
+
+pkg_apocrypha(){
+	# This function begins with "pkg_", but isn't defined in the PMS.
+	local foo=bar/$ROOT
+}
+
+washington_irving(){
+	# This function is arbitrarily-named and not defined in the PMS.
+	local foo="${ROOT}/foo"
+}
+
+# And I suppose we should check that it's not used in global scope, too.
+DEPEND="sys-libs${ROOT}glibc"
-- 
2.7.3

Re: [gentoo-portage-dev] [PATCH 1/1] Add a test case for PMS-compliant usage of the ROOT variable.

[Mike Gilbert]

On Wed, May 11, 2016 at 12:08 PM, Michael Orlitzky <mjo@gentoo.org> wrote:
> A recent thread on gentoo-dev proposed a change to the devmanual's
> description of the ROOT variable:
>
> https://archives.gentoo.org/gentoo-dev/message/8901669dd375ca0fdb610efef0ddfe6f
>
> The proposed change would bring the devmanual's language in line with
> the PMS. That discussion reveals that the use of ROOT outside of
> pkg_* is illegal, yet current versions of repoman/portage allow it.
> This commit adds a test ebuild that violates the PMS (with respect to
> ROOT) in several ways.

This looks good to me, very thorough. I say go ahead and push it.

Re: [gentoo-portage-dev] [PATCH 1/1] Add a test case for PMS-compliant usage of the ROOT variable.

[Brian Dolbec]

On Wed, 11 May 2016 12:27:10 -0400
Mike Gilbert <floppym@gentoo.org> wrote:

> On Wed, May 11, 2016 at 12:08 PM, Michael Orlitzky <mjo@gentoo.org>
> wrote:
> > A recent thread on gentoo-dev proposed a change to the devmanual's
> > description of the ROOT variable:
> >
> > https://archives.gentoo.org/gentoo-dev/message/8901669dd375ca0fdb610efef0ddfe6f
> >
> > The proposed change would bring the devmanual's language in line
> > with the PMS. That discussion reveals that the use of ROOT outside
> > of pkg_* is illegal, yet current versions of repoman/portage allow
> > it. This commit adds a test ebuild that violates the PMS (with
> > respect to ROOT) in several ways.  
> 
> This looks good to me, very thorough. I say go ahead and push it.
> 

I agree, and thank you.

I'll work on adding this check to repoman after I finish getting some
in progress changes done so the new repoman code can be released.

-- 
Brian Dolbec <dolsen>

Re: [gentoo-portage-dev] [PATCH 1/1] Add a test case for PMS-compliant usage of the ROOT variable.

[Michael Orlitzky]

On 05/11/2016 12:33 PM, Brian Dolbec wrote:
> 
> I'll work on adding this check to repoman after I finish getting some
> in progress changes done so the new repoman code can be released.
> 

I took a look at the new code and it was really easy to get something
working. I added a new QA category, "variable.illegal", and left it an
error. People can --force it through if they want to, so I don't think
maintaining backwards-compatibility (against what the PMS says) is crucial.

Adding a new check in ebuild/checks.py was simple: add a PhaseCheck with
a whitelist of phases and perform the check in any other scope.

The only tricky part is the regular expression. The dumbest thing that
could possibly work is,

  rootvar_re = re.compile(r'\$(ROOT|{ROOT})')

That will throw false positives in all sorts of situations, like
app-eselect/eselect-rails-0.21:

  src_prepare() {
    # Fix/Add Prefix support
    sed -i -e 's/\${ROOT}/${EROOT}/' *.eselect || die
  }

The only check I saw that tried to be clever about string parsing is the
variable quoting check. Both seem to share a common need, to perform a
check only when something really is a variable and not part of a string
or otherwise cleverly-disguised.

Should we try to factor something out, or do you want me to send the
naive version (based on the repoman branch) and fix it later?