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.

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

Use a recursive iglob for filesets #765

Merged

Conversation

sergiusens
Copy link
Collaborator

This adds support for ** in filesets for parts.

LP: #1616464

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

This adds support for `**` in filesets for parts.

LP: #1616464

Signed-off-by: Sergio Schvezov <sergio.schvezov@ubuntu.com>
@come-maiz
Copy link
Contributor

retest this please

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

@come-maiz
Copy link
Contributor

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

@sergiusens
Copy link
Collaborator Author

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 ;-)

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

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.

@kyrofa
Copy link
Contributor

kyrofa commented Aug 30, 2016

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

@sergiusens sergiusens merged commit 2d7fc11 into canonical:master Aug 30, 2016
@sergiusens sergiusens deleted the bugfix/1616464/better-fileset-globbing branch August 30, 2016 14:04
@ghost
Copy link

ghost commented Sep 22, 2016

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
Copy link
Collaborator Author

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?

@ghost
Copy link

ghost 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
Copy link
Collaborator Author

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

@ghost
Copy link

ghost 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
Copy link
Collaborator Author

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.

kalikiana pushed a commit to kalikiana/snapcraft that referenced this pull request Apr 6, 2017
This makes use of `**` in filesets for parts.

LP: #1616464

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

Successfully merging this pull request may close these issues.

None yet

3 participants