/snapcraft

Use a recursive iglob for filesets #765

Merged
merged 6 commits into from Aug 30, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@sergiusens @elopio @kyrofa @SamYaple
@sergiusens
Collaborator

sergiusens commented Aug 29, 2016

This adds support for ** in filesets for parts.

LP: #1616464

Signed-off-by: Sergio Schvezov sergio.schvezov@ubuntu.com
@sergiusens
Use a recursive iglob for filesets 
This adds support for `**` in filesets for parts.

LP: #1616464

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>

Some checks were not successful

3 failing and 1 successful checks
@snappy-m-o
autopkgtest integration
Details
@snappy-m-o
autopkgtest snaps
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.02%) to 97.851%
Details
cb2265a

@sergiusens sergiusens referenced this pull request Aug 29, 2016

Closed

Use in plugin code to remove unwanted files. #754

sergiusens added some commits Aug 29, 2016

@sergiusens
integration test

Some checks were not successful

3 failing and 1 successful checks
@snappy-m-o
autopkgtest integration
Details
@snappy-m-o
autopkgtest snaps
Details
continuous-integration/travis-ci/pr — The Travis CI build failed
Details
coverage/coveralls — Coverage increased (+0.02%) to 97.851%
Details
6148f6c
@sergiusens
remove empty

Some checks were not successful

2 failing and 2 successful checks
@snappy-m-o
autopkgtest integration
Details
@snappy-m-o
autopkgtest snaps
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 97.851%
Details
6417d04
@elopio
Member

elopio commented Aug 29, 2016

retest this please
@sergiusens
Merge branch 'master' into bugfix/1616464/better-fileset-globbing

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 97.852%
Details
4d00a59
snapcraft/plugins/autotools.py
- os.unlink(os.path.join(root, file_name))
+ def snap_fileset(self):
+ fileset = super().snap_fileset()
+ fileset.append('-**/*.la')
@elopio

elopio Aug 30, 2016

Member

Please copy the comment:

Remove .la files which don't work when they are moved around

snapcraft/plugins/python2.py
- fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*.pyc')
- fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*/*.pyc')
+ fileset.append('-**/*.pth')
+ fileset.append('-**/*.pyc')
@elopio

elopio Aug 30, 2016

Member

A coment on why are you removing them would be nice here too.

snapcraft/plugins/python3.py
- fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/__pycache__/*.pyc')
- fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*/__pycache__/*.pyc')
+ fileset.append('-**/*.pth')
+ fileset.append('-**/__pycache__')
@elopio

elopio Aug 30, 2016

Member

and here.

@elopio
Member

elopio commented Aug 30, 2016

👍
I worry about @kyrofa, he was so happy about the other branch.
@sergiusens
Collaborator

sergiusens commented Aug 30, 2016

El 30/08/16 a las 00:49, Leo Arias escribió:

👍
I worry about @kyrofa https://github.com/kyrofa, he was so happy
about the other branch.

He said he was going to be happy with this one as well ;-)

sergiusens added some commits Aug 30, 2016

@sergiusens
add code comments for filesets
7ad6a05
@sergiusens
Merge branch 'bugfix/1616464/better-fileset-globbing' of github.com:s… 
…ergiusens/snapcraft into bugfix/1616464/better-fileset-globbing

All checks have passed

4 successful checks
@snappy-m-o
autopkgtest integration — Success
Details
@snappy-m-o
autopkgtest snaps — Success
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.02%) to 97.852%
Details
2494aee
integration_tests/test_python_plugin.py
+ pth_files.extend([f for f in files if f.endswith('pth')])
+
+ self.assertEqual([], pyc_files)
+ self.assertEqual([], pth_files)
@kyrofa

kyrofa Aug 30, 2016 •
Edited 1 time

Member

If this fails, it might be much easier to digest an error based on the length of the lists rather than directly comparing their contents (it'll print the lists, right?). You could still print the list in the failure message if you wanted.

snapcraft/internal/pluginhandler.py
@@ -756,7 +756,8 @@ def _generate_include_set(directory, includes):
include_files = set()
for include in includes:
if '*' in include:
- matches = glob.glob(os.path.join(directory, include))
+ pattern = os.path.join(directory, include)
+ matches = iglob(pattern, recursive=True)
@kyrofa

kyrofa Aug 30, 2016

Member

glob.glob also accepts the recursive param. Is the switch to iglob performance-related?

@sergiusens

sergiusens Aug 30, 2016

Collaborator

@kyrofa it should be. We go over the list only when making it a set on the line below

@kyrofa

kyrofa Aug 30, 2016

Member

Which means you evaluate them all immediately anyway, right? I don't see the difference between this and glob for our usage anyway. That said, I don't have a problem with iglob, I was just curious for the reasoning behind the switch.

snapcraft/plugins/python2.py
+ # We use PYTHONPATH for everything so not needed.
+ fileset.append('-**/*.pth')
+ # This is a major cause of inter part conflict.
+ fileset.append('-**/*.pyc')
@kyrofa

kyrofa Aug 30, 2016

Member

This actually pleases me more than the previous PR, as I don't feel like we're fighting snapcraft here.

snapcraft/plugins/python3.py
+ fileset.append('-**/*.pth')
+ # Holds all the .pyc files. It is a major cause of inter part
+ # conflict.
+ fileset.append('-**/__pycache__')
@kyrofa

kyrofa Aug 30, 2016

Member

Yes, lovely indeed. Thank you for the comments as well.

@kyrofa
Member

kyrofa commented Aug 30, 2016

Very nice! I made one ignorable suggestion, 👍 from me.

@sergiusens sergiusens merged commit 2d7fc11 into snapcore:master Aug 30, 2016
4 checks passed

4 checks passed

autopkgtest integration Success
Details
autopkgtest snaps Success
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.02%) to 97.852%
Details

@sergiusens sergiusens deleted the sergiusens:bugfix/1616464/better-fileset-globbing branch Aug 30, 2016

@SamYaple
Contributor

SamYaple commented Sep 22, 2016 •
Edited 1 time

So this is actually a problem. Specifically with the .pth removal. There are several projects that I know of that have the .pth to (ab)use python importing. Here is a line from the dogpile.cache pth file that allows it to import from site-packages/dogpile/cache without having site-packages/dogpile/init.py

import sys, types, os;p = os.path.join(sys._getframe(1).f_locals['sitedir'], *('dogpile',));ie = os.path.exists(os.path.join(p,'__init__.py'));m = not ie and sys.modules.setdefault('dogpile', types.ModuleType('dogpile'));mp = (m or []) and m.__dict__.setdefault('__path__',[]);(p not in mp) and mp.append(p)

there are many such projects that do things like this unfortunately.
@sergiusens
Collaborator

sergiusens commented Sep 22, 2016

El 22/09/16 a las 13:44, Sam Yaple escribió:

So this is actually a problem. Specifically with the .pth removal.
There are several projects that I know of that have the .pth to
(ab)use python importing. Here is a line from the dogpile.cache pth
file that allows it to import from site-packages/dogpile/cache without
having site-pacakges/dogpile/init.py

import sys, types, os;p =
os.path.join(sys./getframe(1).f_locals['sitedir'], _('dogpile',));ie =
os.path.exists(os.path.join(p,'_init/.py'));m = not ie and
sys.modules.setdefault('dogpile', types.ModuleType('dogpile'));mp = (m
or []) and m.dict.setdefault('_path*',[]);(p not in mp) and mp.append(p)

there are many such projects that do things like this unfortunately.

In your snapcraft.yaml, can you try to explicitly add it?
@SamYaple
Contributor

SamYaple commented Sep 22, 2016

Thats a silly thing to suggest. This was rather complicated to even figure out why it was broken. Even though I can add this file specifically in, this will lead many people to believe that python does not work well with snapcraft.

Indeed, looking through the IRC logs show several cases where people are complaining about it not being able to import a package after this patch merged, I would bet at least some of those are this exact issue.

Why is removing the pth file something that is even needed?
@sergiusens
Collaborator

sergiusens commented Sep 22, 2016

El 22/09/16 a las 14:25, Sam Yaple escribió:

Thats a silly thing to suggest. This was rather complicated to even
figure out why it was broken. Even though I can add this file
specifically in, this will lead many people to believe that python
does not work well with snapcraft.

I am not saying we should keep it this way! :-)

Indeed, looking through the IRC logs show several cases where people
are complaining about it not being able to import a package after this
patch merged, I would bet at least some of those are this exact issue.

Why is removing the pth file something that is even needed?

I think this was just oversight
@SamYaple
Contributor

SamYaple commented Sep 22, 2016

Submitting a patch :)

Not a big deal in the grand scheme of things. Shame i just missed 2.18 window though
@sergiusens
Collaborator

sergiusens commented Sep 22, 2016

El 22/09/16 a las 14:29, Sam Yaple escribió:

Submitting a patch :)

Not a big deal in the grand scheme of things. Shame i just missed 2.18
window though

don't worry, I am preparing 2.18.1 right now.

@SamYaple SamYaple referenced this pull request Sep 22, 2016

Merged

Don't filter .pth files in python plugin #822

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

@sergiusens
Use a recursive iglob for filesets (#765) 
This makes use of `**` in filesets for parts.

LP: #1616464

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
93c82f5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment