Merge lp:~smoser/curtin/trunk.snap-curtin-helpers into lp:~curtin-dev/curtin/trunk

[Scott Moser]

pack: fix packing when curtin is installed inside a snap.

When curtin is installed as part of a MAAS snap, then it would fail
to find its helpers directory. That is because instead of helpers
living in usr/lib/curtin/helpers they will be in
 /snap/maas/<version>/usr/lib/curtin/helpers
But helpfully, SNAP will be set in the environment to point to
 /snap/maas/<version>/

The fix is to adjust pack to consider trust the environment variable
and pack up the installed files based on that prefix.

[Ryan Harper (raharper)]

Looks reasonable, question on 'is None' vs if not foo.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:549
https://jenkins.ubuntu.com/server/job/curtin-ci/694/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/694
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/694
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/694
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/694

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/694/rebuild

review: Approve (continuous-integration)

[lp:~smoser/curtin/trunk.snap-curtin-helpers]

550. By Scott Moser on 2017-12-07

update per ryan feedback

551. By Scott Moser on 2017-12-07

merge with trunk

[Scott Moser (smoser)]

dropped the rpint, and re-ordered the variables.

i really think it makes sense to not change the check for None to false-ish.

its more epxlicit as None, and we're not aware of any uses that are failing.
at very least its not related to this change, so no reason to shove such a change in.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:551
https://jenkins.ubuntu.com/server/job/curtin-ci/695/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/695
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/695
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/695
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/695

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/695/rebuild

review: Approve (continuous-integration)

[Ryan Harper (raharper)]

Hrm,

as with most of these places, it's unlikely that someone ever means to pass
in a '' value here.

These are paths;they're meant to be non-empty strings. At least raise an
exception if the
string value is empty, otherwise aren't we just letting it blow up in some
other way?

Usually it's best practice to fail as soon as we know it's going to break.

On Thu, Dec 7, 2017 at 10:16 AM, Server Team CI bot <
<email address hidden>> wrote:

> Review: Approve continuous-integration
>
> PASSED: Continuous integration, rev:551
> https://jenkins.ubuntu.com/server/job/curtin-ci/695/
> Executed test runs:
> SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=
> metal-amd64/695
> SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=
> metal-arm64/695
> SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=
> metal-ppc64el/695
> SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=
> metal-s390x/695
>
> Click here to trigger a rebuild:
> https://jenkins.ubuntu.com/server/job/curtin-ci/695/rebuild
>
> --
> https://code.launchpad.net/~smoser/curtin/trunk.snap-
> curtin-helpers/+merge/334852
> Your team curtin developers is requested to review the proposed merge of
> lp:~smoser/curtin/trunk.snap-curtin-helpers into lp:curtin.
>

[Ryan Harper (raharper)]

I do think we should fix those; I know our current path doesn't break; but if we're touching it I think it makes sense to raise exceptions when we know we have bad input.

review: Needs Fixing

[Scott Moser (smoser)]

I disagree on two points.
a.) the existing interface is fine and consistent.
If you pass a value for a parameter, then you change behavior for that.
These things do what you want:
 get_paths()
 get_paths(lib="my-lib")
 get_paths(lib=None)

If you pass non-default you get non-default behavior. that is quite common.

b.) I don't agree with changing unrelated behaviors when fixing bugs. You are welcome to suggest that in a merge proposal of your own, but behavior changes do not belong as "tag alongs".

I'm willing to just do this because I'm annoyed by arguing, though.

[Ryan Harper (raharper)]

review: Approve

=== modified file 'curtin/util.py'
--- curtin/util.py	2017-11-29 15:58:25 +0000
+++ curtin/util.py	2017-12-07 15:59:37 +0000
@@ -54,8 +54,8 @@
 
 from .log import LOG
 
-_INSTALLED_HELPERS_PATH = '/usr/lib/curtin/helpers'
-_INSTALLED_MAIN = '/usr/bin/curtin'
+_INSTALLED_HELPERS_PATH = 'usr/lib/curtin/helpers'
+_INSTALLED_MAIN = 'usr/bin/curtin'
 
 _LSB_RELEASE = {}
 _USES_SYSTEMD = None
@@ -677,6 +677,24 @@
     return None
 
 
+def _installed_file_path(path, check_file=None):
+    # check the install root for the file 'path'.
+    #  if 'check_file', then path is a directory that contains file.
+    # return absolute path or None.
+    inst_pre = "/"
+    if os.environ.get('SNAP'):
+        inst_pre = os.path.abspath(os.environ['SNAP'])
+    inst_path = os.path.join(inst_pre, path)
+    if check_file:
+        check_path = os.path.sep.join((inst_path, check_file))
+    else:
+        check_path = inst_path
+
+    if os.path.isfile(check_path):
+        return os.path.abspath(inst_path)
+    return None
+
+
 def get_paths(curtin_exe=None, lib=None, helpers=None):
     # return a dictionary with paths for 'curtin_exe', 'helpers' and 'lib'
     # that represent where 'curtin' executable lives, where the 'curtin' module
@@ -698,17 +716,17 @@
         if found:
             curtin_exe = found
 
-    if (curtin_exe is None and os.path.exists(_INSTALLED_MAIN)):
-        curtin_exe = _INSTALLED_MAIN
+    if curtin_exe is None:
+        curtin_exe = _installed_file_path(_INSTALLED_MAIN)
 
-    cfile = "common"  # a file in 'helpers'
+    # "common" is a file in helpers
+    cfile = "common"
     if (helpers is None and
             os.path.isfile(os.path.join(tld, "helpers", cfile))):
         helpers = os.path.join(tld, "helpers")
 
-    if (helpers is None and
-            os.path.isfile(os.path.join(_INSTALLED_HELPERS_PATH, cfile))):
-        helpers = _INSTALLED_HELPERS_PATH
+    if helpers is None:
+        helpers = _installed_file_path(_INSTALLED_HELPERS_PATH, cfile)
 
     return({'curtin_exe': curtin_exe, 'lib': mydir, 'helpers': helpers})