/snapcraft

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

Merged
merged 11 commits into from Feb 3, 2017
+289 −118

Conversation

Reviewers

@chipaca chipaca

@kyrofa kyrofa

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@sergiusens @battlemidget @kyrofa @chipaca
@sergiusens
Collaborator

sergiusens commented Jan 30, 2017

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

This was referenced Jan 30, 2017

Closed
Closed
@battlemidget

battlemidget commented Jan 30, 2017

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

sergiusens commented Jan 30, 2017

@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]
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)
@chipaca

chipaca Jan 30, 2017

Member

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

@sergiusens

sergiusens Jan 30, 2017

Collaborator

it is sanitized but good idea

snapcraft/internal/meta.py
+ '$LD_LIBRARY_PATH\n')
+ if cwd:
+ f.write('{}\n'.format(cwd))
+ f.write('exec {} {}\n'.format(executable, args))
@chipaca

chipaca Jan 30, 2017

Member

your use of f.write is entertaining :-)

@sergiusens

sergiusens Jan 30, 2017

Collaborator

I can change to print 😉

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)
@chipaca

chipaca Jan 30, 2017

Member

# leftover? Or ...?

@sergiusens

sergiusens Jan 30, 2017

Collaborator

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

@kyrofa

kyrofa requested changes Jan 31, 2017
View changes

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.

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))
@kyrofa

kyrofa Jan 31, 2017

Member

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

snapcraft/internal/meta.py
+ '$LD_LIBRARY_PATH', file=f)
+ if cwd:
+ print('{}'.format(cwd), file=f)
+ print('exec {} {}'.format(executable, args), file=f)
@kyrofa

kyrofa Jan 31, 2017

Member

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

@sergiusens

sergiusens Feb 1, 2017

Collaborator

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

@kyrofa

kyrofa Feb 1, 2017

Member

Indeed it does. Feel free to ignore.

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
@kyrofa

kyrofa Jan 31, 2017

Member

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.

@sergiusens

sergiusens Feb 1, 2017

Collaborator

this can be clarified

snapcraft/plugins/python.py
+
+ env['PATH'] = '{}:{}'.format(
+ os.path.join(self.installdir, 'usr', 'bin'),
+ os.path.expandvars('$PATH'))
@kyrofa

kyrofa Jan 31, 2017

Member

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

snapcraft/shutil.py
+from snapcraft.internal import common
+
+
+def which(command, **kwargs):
@kyrofa

kyrofa Jan 31, 2017

Member

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.

@sergiusens

sergiusens Feb 1, 2017

Collaborator

this is a common practice though.

@sergiusens

sergiusens Feb 1, 2017

Collaborator

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

@sergiusens

sergiusens Feb 1, 2017

Collaborator

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

@battlemidget

battlemidget commented Feb 1, 2017 •
Edited 1 time

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

sergiusens added some commits Jan 26, 2017

@sergiusens
python plugin: do the right thing with classic. 
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>
a82304d
@sergiusens
simplify pip calls
c35840c
@sergiusens
escape parts_dir
814c815
@sergiusens
use print
319920c
@sergiusens
make stage_packages a property to make it dynamic for the python plugin
57d5fdd
@sergiusens
support chaining python coming from stage-packages
5df813b
@sergiusens
readd filesets exclusion, we only need the modules
5da2489
@sergiusens
fix tests to use the new shebang
d974682
@sergiusens
review

Some checks were not successful

1 failing and 2 successful checks
@snappy-m-o
xenial-amd64 — autopkgtest finished (failure)
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@snappy-m-o
zesty-amd64 — autopkgtest finished (success)
Details
9adcfb5
@sergiusens
better comment

All checks have passed

3 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@snappy-m-o
xenial-amd64 — autopkgtest finished (success)
Details
@snappy-m-o
zesty-amd64 — autopkgtest finished (success)
Details
a784524
@battlemidget

battlemidget commented Feb 2, 2017

I've successfully tested this branch and was able to build https://github.com/conjure-up/conjure-up on Trusty with huge success.
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)
@kyrofa

kyrofa Feb 2, 2017

Member

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

@sergiusens

sergiusens Feb 2, 2017

Collaborator

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

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)
@kyrofa

kyrofa Feb 2, 2017

Member

Right here too.

@sergiusens
oh kyle

All checks have passed

3 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
@snappy-m-o
xenial-amd64 — autopkgtest finished (success)
Details
@snappy-m-o
zesty-amd64 — autopkgtest finished (success)
Details
ddc32b1
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.
@kyrofa

kyrofa Feb 2, 2017

Member

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

@kyrofa

kyrofa approved these changes Feb 2, 2017
View changes

Nice job on this, looks great!

@kyrofa kyrofa merged commit e782409 into snapcore:master Feb 3, 2017
3 checks passed

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
xenial-amd64 autopkgtest finished (success)
Details
zesty-amd64 autopkgtest finished (success)
Details

elopio added a commit that referenced this pull request Feb 28, 2017

@sergiusens @elopio
python plugin: do the right thing with classic. (#1093) 
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>
9345a4e

@sergiusens sergiusens deleted the sergiusens:bugfix/classic-python-plugin branch Mar 16, 2017

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017

@sergiusens @kyrofa
python plugin: do the right thing with classic. (#1093) 
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>
1ad0f27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment