python plugin: do the right thing with classic. #1093

sergiusens · 2017-01-30T16:54:40Z

To fully enable classic confinment for python based snaps, there are
two big things that need to get done; no env var leaking and
use a python interpreter with the classic compilation flags.

LP: #1657504

Signed-off-by: Sergio Schvezov sergio.schvezov@canonical.com

adam-stokes · 2017-01-30T16:56:19Z

Will this allow a successful installation of snaps on trusty as well?

sergiusens · 2017-01-30T17:27:21Z

@battlemidget yes; there is only one last thing for trusty support being tracked on https://bugs.launchpad.net/bugs/1657504

There is however one last issue involving core on comment https://bugs.launchpad.net/snapd/+bug/1657504/comments/15

For classic, you will need to provide an interpreter, a quick way is like

name: asciinema
version: '1.3.0'
summary: Record and share your terminal sessions, the right way.
description: |
  Forget screen recording apps and blurry video. Enjoy a lightweight,
  purely text-based approach to terminal recording.

grade: devel
confinement: classic

apps:
  asciinema:
    command: bin/asciinema

parts:
  asciinema:
    plugin: python
    python-packages: [$SNAPCRAFT_PROJECT_NAME==$SNAPCRAFT_PROJECT_VERSION]
    after: [python3]
  python3:
    source: https://www.python.org/ftp/python/3.6.0/Python-3.6.0.tar.xz
    plugin: autotools
    configflags: [--prefix=/usr]
    build-packages: [libssl-dev]

chipaca · 2017-01-30T17:45:13Z

snapcraft/internal/meta.py

+        # binaries are linked with `nodefaultlib` but we still do
+        # not want to leak PATH or other environment variables
+        # that would affect the applications view of the classic
+        # environment it is dropped into.
        replace_path = r'{}/[a-z0-9][a-z0-9+-]*/install'.format(
            self._parts_dir)


has _parts_dir been sanitized to not have regexpish things in it? otherwise you want re.escape(self._parts_dir) here

it is sanitized but good idea

chipaca · 2017-01-30T17:46:20Z

snapcraft/plugins/python.py

                   package_dir=self._python_package_dir, env=env,
                   extra_install_args=['--ignore-installed'])

        if download:
            pip.download(args)
-        pip.wheel(args)
+        # pip.wheel(args)


# leftover? Or ...?

yeah, already removed locally, didn't want to stress the weak infra ;-)

chipaca · 2017-01-30T17:48:54Z

snapcraft/internal/meta.py

+                        '$LD_LIBRARY_PATH\n')
+            if cwd:
+                f.write('{}\n'.format(cwd))
+            f.write('exec {} {}\n'.format(executable, args))


your use of f.write is entertaining :-)

I can change to print 😉

kyrofa

This looks mostly good to me. I'll be honest and say I'm not familiar with the site package fiddling we're doing here, but I looked into it and it seems sane. I have a few niggly comments, the bigger one is the shutil.which() duplication that's happening here. In addition to not being sure if it's necessary, I'm worried about naming confusion.

kyrofa · 2017-01-31T22:06:15Z

snapcraft/internal/meta.py

        replace_path = r'{}/[a-z0-9][a-z0-9+-]*/install'.format(
-            self._parts_dir)
-        assembled_env = re.sub(replace_path, '$SNAP', assembled_env)
+            re.escape(self._parts_dir))


This is used a few times. re.compile() might be a good idea.

kyrofa · 2017-01-31T22:11:46Z

snapcraft/internal/meta.py

+                      '$LD_LIBRARY_PATH', file=f)
+            if cwd:
+                print('{}'.format(cwd), file=f)
+            print('exec {} {}'.format(executable, args), file=f)


Why not just collect all the lines in a list and finally f.write('\n'.join(lines)) at the end?

I guess this boils down to personal preference, doesn't it?

Indeed it does. Feel free to ignore.

kyrofa · 2017-01-31T22:23:09Z

snapcraft/plugins/python.py


        args = ['pip', 'setuptools', 'wheel']

+        pip_command = [self._get_python_command(), '-m', 'pip']
+
+        # if python_command it is not from stage we don't have pip


s/if python_command it is/if python_command is/. Although from the comment I'm not sure how that relates to the setting of PYTHONHOME.

this can be clarified

kyrofa · 2017-01-31T22:26:57Z

snapcraft/plugins/python.py

+
+        env['PATH'] = '{}:{}'.format(
+            os.path.join(self.installdir, 'usr', 'bin'),
+            os.path.expandvars('$PATH'))


Huh, I didn't know about os.path.expandvars(), nice.

kyrofa · 2017-01-31T22:29:56Z

snapcraft/shutil.py

+from snapcraft.internal import common
+
+
+def which(command, **kwargs):


Why not use the real shutil.which() with assemble_env()'s PATH? If we want to keep this, can we name the file something else? I'm afraid shutil will cause confusion later.

this is a common practice though.

also, we want the specific env to take precedence (PATH)

assembled_env's PATH is a list with multiple entries of PATH

adam-stokes · 2017-02-01T21:52:44Z

I attempted to get my conjure-up snap working with this branch on trusty. This is the packaged snap contents of that:

http://paste.ubuntu.com/23907436/

I was able to install without an issue this time, however, here is the error i received:

/snap/conjure-up/x1/wrappers/conjure-up: 14: exec: /snap/conjure-up/x1/bin/conjure-up: not found

which goes inline with the contents of the snap.

On a plus though, my original issue with the systemd services not being installed have been resolved \o/

For reference here is my snapcraft im using https://github.com/conjure-up/conjure-up/blob/master/snap/snapcraft.yaml

Update

After adding the python part the build succeeded, however, I am getting another error when trying to start the systemd service:

ubuntu@darthbawlz:~$ systemctl status snap.conjure-up.bridge.service
Failed to issue method call: No such interface 'org.freedesktop.DBus.Properties' on object at path /org/freedesktop/systemd1/unit/snap_2econjure_2dup_2ebridge_2eservice

To fully enable classic confinment for python based snaps, there are two big things that need to get done; no env var leaking and use a python interpreter with the classic compilation flags. LP: #1657504 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

adam-stokes · 2017-02-02T21:54:11Z

I've successfully tested this branch and was able to build https://github.com/conjure-up/conjure-up on Trusty with huge success.

kyrofa · 2017-02-02T21:57:09Z

snapcraft/internal/meta.py

+        else:
+            assembled_env = common.assemble_env()
+            assembled_env = assembled_env.replace(self._snap_dir, '$SNAP')
+            assembled_env = re.sub(replace_path, '$SNAP', assembled_env)


You probably want replace_path.sub('$SNAP', assembled_env) here, no?

ah, you should of been more specific; the current mechanism works though :-P

kyrofa · 2017-02-02T21:59:03Z

snapcraft/internal/meta.py

+
+        if shebang:
+            if shebang.startswith('/usr/bin/env '):
+                shebang = shell_utils.which(shebang.split()[1])
            new_shebang = re.sub(replace_path, '$SNAP', shebang)


Right here too.

kyrofa · 2017-02-02T22:00:44Z

snapcraft/plugins/python.py

+        # If python_command it is not from stage we don't have pip, which means
+        # we are going to need to resort to the pip installed on the system
+        # that came from build-packages. This shouldn't be a problem as
+        # stage-packages and build-packages should match.


👍 thank you. Though the typo is still there: s/python_command it is not/python_command is not/

kyrofa

Nice job on this, looks great!

To fully enable classic confinement for python based snaps, there are two big things that need to get done; no env var leaking and use a python interpreter with the classic compilation flags. LP: #1657504 Signed-off-by: Sergio Schvezov <sergio.schvezov@canonical.com>

This was referenced Jan 30, 2017

python plugin: avoid the use of PYTHON* env vars. #1080

Closed

core: classic with no exported variables. #1090

Closed

sergiusens force-pushed the bugfix/classic-python-plugin branch 2 times, most recently from 494b754 to 57d7255 Compare January 30, 2017 17:24

chipaca reviewed Jan 30, 2017

View reviewed changes

sergiusens force-pushed the bugfix/classic-python-plugin branch from 11469f6 to e2f26c7 Compare January 31, 2017 13:45

kyrofa suggested changes Jan 31, 2017

View reviewed changes

sergiusens force-pushed the bugfix/classic-python-plugin branch from b58e16c to 73425ee Compare February 1, 2017 18:06

sergiusens added 9 commits February 2, 2017 21:12

simplify pip calls

c35840c

escape parts_dir

814c815

use print

319920c

make stage_packages a property to make it dynamic for the python plugin

57d5fdd

support chaining python coming from stage-packages

5df813b

readd filesets exclusion, we only need the modules

5da2489

fix tests to use the new shebang

d974682

review

9adcfb5

sergiusens force-pushed the bugfix/classic-python-plugin branch from 73425ee to 9adcfb5 Compare February 2, 2017 21:46

better comment

a784524

kyrofa reviewed Feb 2, 2017

View reviewed changes

oh kyle

ddc32b1

kyrofa reviewed Feb 2, 2017

View reviewed changes

kyrofa approved these changes Feb 2, 2017

View reviewed changes

kyrofa merged commit e782409 into canonical:master Feb 3, 2017

sergiusens deleted the bugfix/classic-python-plugin branch March 16, 2017 20:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

python plugin: do the right thing with classic. #1093

python plugin: do the right thing with classic. #1093

sergiusens commented Jan 30, 2017

adam-stokes commented Jan 30, 2017

sergiusens commented Jan 30, 2017

chipaca Jan 30, 2017

sergiusens Jan 30, 2017

chipaca Jan 30, 2017

sergiusens Jan 30, 2017

chipaca Jan 30, 2017

sergiusens Jan 30, 2017

kyrofa left a comment

kyrofa Jan 31, 2017

kyrofa Jan 31, 2017

sergiusens Feb 1, 2017

kyrofa Feb 1, 2017

kyrofa Jan 31, 2017

sergiusens Feb 1, 2017

kyrofa Jan 31, 2017

kyrofa Jan 31, 2017

sergiusens Feb 1, 2017

sergiusens Feb 1, 2017

sergiusens Feb 1, 2017

adam-stokes commented Feb 1, 2017 •

edited

adam-stokes commented Feb 2, 2017

kyrofa Feb 2, 2017

sergiusens Feb 2, 2017

kyrofa Feb 2, 2017

kyrofa Feb 2, 2017

kyrofa left a comment

		from snapcraft.internal import common


		def which(command, **kwargs):

python plugin: do the right thing with classic. #1093

python plugin: do the right thing with classic. #1093

Conversation

sergiusens commented Jan 30, 2017

adam-stokes commented Jan 30, 2017

sergiusens commented Jan 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adam-stokes commented Feb 1, 2017 • edited

adam-stokes commented Feb 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyrofa left a comment

Choose a reason for hiding this comment

adam-stokes commented Feb 1, 2017 •

edited