Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use a recursive iglob for filesets #765

Merged
merged 6 commits into from Aug 30, 2016
Merged

Use a recursive iglob for filesets #765

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
cb2265a
Use a recursive iglob for filesets
sergiusens Aug 29, 2016
6148f6c
integration test
sergiusens Aug 29, 2016
6417d04
remove empty
sergiusens Aug 29, 2016
4d00a59
Merge branch 'master' into bugfix/1616464/better-fileset-globbing
sergiusens Aug 30, 2016
7ad6a05
add code comments for filesets
sergiusens Aug 30, 2016
2494aee
Merge branch 'bugfix/1616464/better-fileset-globbing' of github.com:s…
sergiusens Aug 30, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
18 changes: 18 additions & 0 deletions integration_tests/test_python_plugin.py
View file
Open in desktop
Expand Up @@ -83,6 +83,24 @@ def test_build_rewrites_shebangs(self):
self.assertEqual('#!/usr/bin/env python2', python2_shebang)
self.assertEqual('#!/usr/bin/env python3', python3_shebang)

def test_build_does_not_keep_pyc_or_pth_files_in_install(self):
# .pyc and .pyc files collide between parts.
# There is no way to tell pip or setup.py to disable generation of
# .pyc
# The .pth files are only relevant if found inside the pre compiled
# site-packges directory so we don't want those either.
project_dir = 'pip-requirements-file'
self.run_snapcraft('stage', project_dir)

pyc_files = []
pth_files = []
for _, _, files in os.walk(os.path.join(project_dir, 'stage')):
pyc_files.extend([f for f in files if f.endswith('pyc')])
pth_files.extend([f for f in files if f.endswith('pth')])

self.assertEqual([], pyc_files)
self.assertEqual([], pth_files)
Copy link
Contributor

@kyrofa kyrofa Aug 30, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


def test_build_doesnt_get_bad_install_directory_lp1586546(self):
"""Verify that LP: #1586546 doesn't come back."""
project_dir = 'python-pyyaml'
Expand Down
8 changes: 5 additions & 3 deletions snapcraft/internal/pluginhandler.py
View file
Open in desktop
Expand Up @@ -16,13 +16,13 @@

import contextlib
import filecmp
import glob
import importlib
import itertools
import logging
import os
import shutil
import sys
from glob import iglob

import jsonschema
import magic
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

include_files |= set(matches)
else:
include_files |= set([os.path.join(directory, include), ])
Expand All @@ -782,7 +783,8 @@ def _generate_exclude_set(directory, excludes):
exclude_files = set()

for exclude in excludes:
matches = glob.glob(os.path.join(directory, exclude))
pattern = os.path.join(directory, exclude)
matches = iglob(pattern, recursive=True)
exclude_files |= set(matches)

exclude_dirs = [os.path.relpath(x, directory)
Expand Down
12 changes: 4 additions & 8 deletions snapcraft/plugins/autotools.py
View file
Open in desktop
Expand Up @@ -124,11 +124,7 @@ def build(self):
self.run(['make', '-j{}'.format(self.parallel_build_count)])
self.run(make_install_command)

# Remove .la files which don't work when they are moved around
self._remove_la_files()

def _remove_la_files(self):
for root, _, files in os.walk(self.installdir):
for file_name in files:
if file_name.endswith('.la'):
os.unlink(os.path.join(root, file_name))
def snap_fileset(self):
fileset = super().snap_fileset()
fileset.append('-**/*.la')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please copy the comment:

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

return fileset
14 changes: 2 additions & 12 deletions snapcraft/plugins/python2.py
View file
Open in desktop
Expand Up @@ -206,18 +206,8 @@ def build(self):
def snap_fileset(self):
fileset = super().snap_fileset()
fileset.append('-usr/bin/pip*')
fileset.append('-usr/lib/python*/dist-packages/easy-install.pth')
fileset.append('-usr/lib/python*/dist-packages/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*.pyc')
fileset.append('-usr/lib/python*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*/*.pyc')
fileset.append('-**/*.pth')
fileset.append('-**/*.pyc')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return fileset


Expand Down
12 changes: 2 additions & 10 deletions snapcraft/plugins/python3.py
View file
Open in desktop
Expand Up @@ -184,14 +184,6 @@ def python_version(self):
def snap_fileset(self):
fileset = super().snap_fileset()
fileset.append('-usr/bin/pip*')
fileset.append('-usr/lib/python*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-usr/lib/python*/*/*/*/*/*/*/*/*/*/__pycache__/*.pyc')
fileset.append('-**/*.pth')
fileset.append('-**/__pycache__')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return fileset
9 changes: 9 additions & 0 deletions snapcraft/tests/test_plugin_autotools.py
View file
Open in desktop
Expand Up @@ -308,3 +308,12 @@ def _run(cmd):
# An exception will be raised if build can't handle the non-executable
# autogen.
plugin.build()

def test_fileset_ignores(self):
plugin = autotools.AutotoolsPlugin('test-part', self.options,
self.project_options)
expected_fileset = [
'-**/*.la',
]
fileset = plugin.snap_fileset()
self.assertListEqual(expected_fileset, fileset)
14 changes: 2 additions & 12 deletions snapcraft/tests/test_plugin_python2.py
View file
Open in desktop
Expand Up @@ -180,18 +180,8 @@ def test_fileset_ignores(self):
self.project_options)
expected_fileset = [
'-usr/bin/pip*',
'-usr/lib/python*/dist-packages/easy-install.pth',
'-usr/lib/python*/dist-packages/__pycache__/*.pyc',
'-usr/lib/python*/*.pyc',
'-usr/lib/python*/*/*.pyc',
'-usr/lib/python*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/*/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/*/*/*.pyc',
'-**/*.pth',
'-**/*.pyc',
]
fileset = plugin.snap_fileset()
self.assertListEqual(expected_fileset, fileset)
Expand Down
12 changes: 2 additions & 10 deletions snapcraft/tests/test_plugin_python3.py
View file
Open in desktop
Expand Up @@ -130,16 +130,8 @@ def test_fileset_ignores(self):
self.project_options)
expected_fileset = [
'-usr/bin/pip*',
'-usr/lib/python*/__pycache__/*.pyc',
'-usr/lib/python*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/*/__pycache__/*.pyc',
'-usr/lib/python*/*/*/*/*/*/*/*/*/*/__pycache__/*.pyc',
'-**/*.pth',
'-**/__pycache__',
]
fileset = plugin.snap_fileset()
self.assertListEqual(expected_fileset, fileset)
Expand Down