Python plugin improvements #771

sergiusens · Aug 31, 2016

The unit tests still need updating, what do people think about this approach?

The demos still work btw.

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

this is not a great idea, conceptually. Python3 is not a parent of python2, so inheritance could cause problems in the future. A better approach might be to make a common BasePythonPlugin, and inherit both Python2 and Python3 plugins from it.

+1, agreed.

and where would BasePythonPlugin live? in the python3 module or the python2 one?

neither. In a common module.

El 01/09/16 a las 11:04, Leo Arias escribió:

In snapcraft/plugins/python2.py
#771 (comment):

logger = logging.getLogger(name)

-class Python2Plugin(snapcraft.BasePlugin):
+class Python2Plugin(python3.Python3Plugin):

neither. In a common module.

I don't know, this gives the plugins we write special treatment, this
module would have to be a plugin as well and we risk having a useless
plugin like the jdk one.

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

you can just remove the if True, and leave the print for now.

yeah, sounds good

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

s/The python plugin/It

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

raise an exception? This should never happen.
Now this is a good use for assert :p

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

is there a way to enforce the two valid values?

El 01/09/16 a las 12:56, Leo Arias escribió:

In snapcraft/plugins/python.py
#771 (comment):

   schema['properties']['python-packages'] = {

```
       'type': 'array',
```
```
       'minitems': 1,
```
```
       'uniqueItems': True,
```
```
       'items': {
```
```
           'type': 'string'
```
```
       },
```
```
       'default': [],
```
```
   }
```

   schema['properties']['process-dependency-links'] = {

```
       'type': 'boolean',
```
```
       'default': False,
```
```
   }
```

   schema['properties']['python-version'] = {

```
       'type': 'string',
```
```
       'default': 'python3',
```

is there a way to enforce the two valid values?

yes I added an enum

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

What about the build properties?
I'm not sure how does this work. If the pull properties are changed, and snapcraft build is executed, will it run pull again?

@kyrofa why was build-properties not originally added here?

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

this too should never happen

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

2015, 2016

El 01/09/16 a las 13:01, Leo Arias escribió:

In snapcraft/plugins/python3.py
#771 (comment):

@@ -1,6 +1,6 @@

-- Mode:Python; indent-tabs-mode:nil; tab-width:4 --

-# Copyright (C) 2015 Canonical Ltd
+# Copyright (C) 2015-2016 Canonical Ltd

2015, 2016

we already had this discussion ;-)

yes, and as far as I remember, I won ^_^

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

print a deprecation warning when it's used?

elopio · Sep 1, 2016

elopio reviewed Sep 1, 2016

View changes

why setattr instead of options.python_version = ... ?

Because it doesn't exist, options gets created from the plugin's schema

elopio · Sep 1, 2016

oh, yes! /me approves.

SamYaple · Sep 1, 2016

SamYaple reviewed Sep 1, 2016

View changes

no reason to do this. just don't pop 'required' from the options schema. enforce the requirement of python_version (and specific values with enum so we dont have to validate it either)

yup, and this could be a dictionary. return self._python_stage_packages[self.options.python_version]

Then an exception would be raised anyway if there is a programmer error somewhere.

the problem is on inheritance. Adding the schema makes the option visible to the python2 and python3 plugins

IOW this is here more for the children than the parent itself

Hmmm. I suppose schema could be added to the children too if that a problem.

I just don't like the idea of checking python versions in multiple places. Perhaps abstract the else to another function so we don't have to change exceptions in multiple places should this need to be handled differently in the future? (I'm seeing potential uses for python3.4 vs python3.5 maybe). I just don't like violating DRY in this way.

Seems like something that can be decorated.

elopio · Sep 1, 2016

I think this is very good. I left some minor ignorable comments, so you'll have my +1...
after you add an integration test running the prime command on a snap with "plugin: python" 😆

SamYaple · Sep 1, 2016

You beat me to it! Glad you also like the approach of python2 and python3 inheriting a 'python' plugin for deprecation. I could think of no other way to do it

sergiusens · Sep 2, 2016

LP: #1616032

sergiusens · Sep 6, 2016

LP: #1616407

sergiusens · Sep 6, 2016

LP: #1606894

sergiusens · Sep 6, 2016

LP: #1602079

sergiusens · Sep 6, 2016

retest this please

sergiusens · Sep 6, 2016

LP: #1594809

sergiusens · Sep 6, 2016

retest this please

sergiusens · Sep 6, 2016

LP: #1607297

elopio · Sep 7, 2016

elopio reviewed Sep 7, 2016

View changes

would you mind explaining in a docstring what is this doing?

SamYaple · Sep 7, 2016

Merge merge merge 👍

Patchset looks good. I have been using it exclusively for a few days now. Possibly some docs/testing stuff that is needed, but functionally it is solid.

elopio · Sep 7, 2016

when trying to snap integration_tests/snaps/pip-requirements-list, it results in a conflict with bin/jsonschema. This is known, right? It's part of the change you commented in the mailing list about entry points?

sergiusens · Sep 7, 2016

El 07/09/16 a las 00:32, Leo Arias escribió:

when trying to snap integration_tests/snaps/pip-requirements-list, it
results in a conflict with bin/jsonschema. This is known, right? It's
part of the change you commented in the mailing list about entry points?

There is a fileset the takes this into account, integration tests pass,
so I don't see what the problem is. This means entry point work now btw!

sergiusens · Sep 7, 2016

LP: #1619104

add enum

All checks have failed
4 failing checks

autopkgtest integration

Details

autopkgtest snaps

Details

continuous-integration/travis-ci/pr — The Travis CI build failed

Details

coverage/coveralls — Coverage decreased (-0.005%) to 97.899%

Details

4e77dfe

pull properties

All checks have failed
3 failing checks

autopkgtest integration

Details

autopkgtest snaps

Details

continuous-integration/travis-ci/pr — The Travis CI build failed

Details

3f6d73d

no if True

All checks have failed
3 failing checks

autopkgtest integration

Details

autopkgtest snaps

Details

continuous-integration/travis-ci/pr — The Travis CI build failed

Details

54749b8

sergiusens self-assigned this Sep 2, 2016

sergiusens deleted the sergiusens:python-plugin-improvements branch Sep 7, 2016

sergiusens referenced this pull request Sep 14, 2016
Closed
python3 plugin: run setup.py in source subdir if such option exists #751

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

Python plugin improvements (#771)
LP: #1594809 LP: #1602079 LP: #1606894 LP: #1607297 LP: #1616407 LP: #1619104 Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

75674ab

@@ -237,17 +237,15 @@ def parallel_build_count(self):
              # Helpers
              def run(self, cmd, cwd=None, **kwargs):
-        -        if cwd is None:
+        +        if not cwd:
                      cwd = self.builddir
                  if True:

+        +                'python-setuptools',
+        +            ]
+        +        else:
+        +            return []

+        +            'constraints',
+        +            'python-packages',
+        +            'python-version',
+        +        ])

+        +        elif self.options.python_version == 'python2':
+        +            return ['python2']
+        +        else:
+        +            return []

+        +        elif self.options.python_version == 'python2':
+        +            return ['python']
+        +        else:
+        +            raise AssertionError('python-version has a value of {!r}'.format(

snapcore/snapcraft

Join GitHub today

Python plugin improvements #771

Conversation

sergiusens commented Aug 31, 2016 • edited Edited 1 time sergiusens Aug 31, 2016

sergiusens added some commits Aug 31, 2016

All checks have failed

Some checks were not successful

All checks have failed

Some checks were not successful

All checks have failed

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

elopio reviewed Sep 1, 2016 View changes

sergiusens added some commits Sep 1, 2016

Some checks were not successful

Some checks were not successful

Some checks were not successful

All checks have failed

elopio reviewed Sep 1, 2016 View changes

elopio reviewed Sep 1, 2016 View changes

All checks have failed

elopio reviewed Sep 1, 2016 View changes

elopio reviewed Sep 1, 2016 View changes

All checks have failed

elopio reviewed Sep 1, 2016 View changes

elopio reviewed Sep 1, 2016 View changes

All checks have failed

elopio reviewed Sep 1, 2016 View changes

-- Mode:Python; indent-tabs-mode:nil; tab-width:4 --

elopio reviewed Sep 1, 2016 View changes

elopio reviewed Sep 1, 2016 View changes

elopio commented Sep 1, 2016

sergiusens added some commits Sep 1, 2016

Some checks were not successful

Some checks were not successful

Some checks were not successful

Some checks were not successful

SamYaple reviewed Sep 1, 2016 View changes

sergiusens Sep 1, 2016 • edited Edited 1 time sergiusens Sep 1, 2016

elopio commented Sep 1, 2016

SamYaple commented Sep 1, 2016

sergiusens added some commits Sep 1, 2016

Some checks were not successful

Some checks were not successful

sergiusens commented Sep 2, 2016

sergiusens self-assigned this Sep 2, 2016

sergiusens added some commits Sep 3, 2016

Some checks were not successful

All checks have passed

All checks have passed

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

sergiusens commented Sep 6, 2016

elopio reviewed Sep 7, 2016 View changes

SamYaple commented Sep 7, 2016

elopio commented Sep 7, 2016

sergiusens commented Sep 7, 2016

sergiusens commented Sep 7, 2016

Hide details View details sergiusens merged commit bb7bd40 into snapcore:master Sep 7, 2016 4 checks passed

4 checks passed

sergiusens deleted the sergiusens:python-plugin-improvements branch Sep 7, 2016

sergiusens referenced this pull request Sep 14, 2016

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

sergiusens commented Aug 31, 2016 •

Edited 1 time

sergiusens
Aug 31, 2016

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

elopio reviewed Sep 1, 2016

View changes

SamYaple reviewed Sep 1, 2016

View changes

sergiusens Sep 1, 2016 •

Edited 1 time

sergiusens
Sep 1, 2016

elopio reviewed Sep 7, 2016

View changes

sergiusens merged commit `bb7bd40` into snapcore:master Sep 7, 2016
4 checks passed