[PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

os.walk() returns symlinks to directories in the "dirs" lists,
but then never enters them by default. As a result, the old
code applied neither the directory handling (because that
is active once a directory gets entered) nor the file handling,
and thus never packaged such symlinks.

The fix is simple: find such special directory entries and move
them to the "files" list.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/classes/package_rpm.bbclass | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index 92ddf7a..6483e96 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -197,6 +197,13 @@ python write_specfile () {
             if path.endswith("DEBIAN") or path.endswith("CONTROL"):
                 continue
 
+            # Treat all symlinks to directories as normal files.
+            # os.walk() lists them as directories.
+            for i, entry in enumerate(dirs):
+                if os.path.islink(os.path.join(rootpath, entry)):
+                    del dirs[i]
+                    files.append(entry)
+
             # Directory handling can happen in two ways, either DIRFILES is not set at all
             # in which case we fall back to the older behaviour of packages owning all their
             # directories
-- 
1.8.4.5

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 790 bytes --]

On Tue, Feb 17, 2015 at 6:42 AM, Patrick Ohly <patrick.ohly@intel.com>
wrote:

> +            # Treat all symlinks to directories as normal files.
> +            # os.walk() lists them as directories.
> +            for i, entry in enumerate(dirs):
> +                if os.path.islink(os.path.join(rootpath, entry)):
> +                    del dirs[i]
> +                    files.append(entry)
> +
>

You're deleting elements of a list while you're iterating over it. I'm
fairly certain that will lead to pain, unless you explicitly ensure you're
operating against a copy: for i, entry in enumerate(list(dirs)):
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 1252 bytes --]

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

On Tue, 2015-02-17 at 08:55 -0700, Christopher Larson wrote:
> 
> On Tue, Feb 17, 2015 at 6:42 AM, Patrick Ohly <patrick.ohly@intel.com>
> wrote:
>         +            # Treat all symlinks to directories as normal
>         files.
>         +            # os.walk() lists them as directories.
>         +            for i, entry in enumerate(dirs):
>         +                if os.path.islink(os.path.join(rootpath,
>         entry)):
>         +                    del dirs[i]
>         +                    files.append(entry)
>         +
> 
> You're deleting elements of a list while you're iterating over it. I'm
> fairly certain that will lead to pain, unless you explicitly ensure
> you're operating against a copy: for i, entry in
> enumerate(list(dirs)):

I was wondering about that myself, but couldn't find any definite
statement about whether it's okay or not for enumerate(). It works in
practice, but of course that doesn't guarantee that it is okay.

Iterating backwards will be more obviously correct, I'll send a patch
update using that.

I'm still curious, though, whether the code above is really broken or
just dubious because one cannot be certain.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

On Wed, 2015-02-18 at 09:43 +0100, Patrick Ohly wrote:
> On Tue, 2015-02-17 at 08:55 -0700, Christopher Larson wrote:
> > 
> > On Tue, Feb 17, 2015 at 6:42 AM, Patrick Ohly <patrick.ohly@intel.com>
> > wrote:
> >         +            # Treat all symlinks to directories as normal
> >         files.
> >         +            # os.walk() lists them as directories.
> >         +            for i, entry in enumerate(dirs):
> >         +                if os.path.islink(os.path.join(rootpath,
> >         entry)):
> >         +                    del dirs[i]
> >         +                    files.append(entry)
> >         +
> > 
> > You're deleting elements of a list while you're iterating over it. I'm
> > fairly certain that will lead to pain, unless you explicitly ensure
> > you're operating against a copy: for i, entry in
> > enumerate(list(dirs)):
> 
> I was wondering about that myself, but couldn't find any definite
> statement about whether it's okay or not for enumerate(). It works in
> practice, but of course that doesn't guarantee that it is okay.

The enumerate() documentation says that it is equivalent to a "for in"
loop, and documentation for that says "it is *recommended* that you
first make a copy" (emphasis mine). IMHO it means the behavior is simply
undefined.

> Iterating backwards will be more obviously correct, I'll send a patch
> update using that.

The recommended approach is using a slice copy
(https://docs.python.org/2/tutorial/controlflow.html#for-statements), so
how about this:

# Avoid modifying the list we iterate over, iterate over slice copy
# instead.
for i, entry in enumerate(dirs[:]):
    if os.path.islink(os.path.join(rootpath, entry)):
         del dirs[i]
         files.append(entry)

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Mark Hatle]

On 2/17/15 7:42 AM, Patrick Ohly wrote:
> os.walk() returns symlinks to directories in the "dirs" lists,
> but then never enters them by default. As a result, the old
> code applied neither the directory handling (because that
> is active once a directory gets entered) nor the file handling,
> and thus never packaged such symlinks.
> 
> The fix is simple: find such special directory entries and move
> them to the "files" list.

I'm curious, what is the problem that is being fixed with this.

Normally you don't want to walk into symlinks to directories, as either you want
to capture the symlink itself -- or the symlink is pointing to something you
don't have access to when packaging (i.e. /proc).

--Mark

> Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
> ---
>  meta/classes/package_rpm.bbclass | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
> index 92ddf7a..6483e96 100644
> --- a/meta/classes/package_rpm.bbclass
> +++ b/meta/classes/package_rpm.bbclass
> @@ -197,6 +197,13 @@ python write_specfile () {
>              if path.endswith("DEBIAN") or path.endswith("CONTROL"):
>                  continue
>  
> +            # Treat all symlinks to directories as normal files.
> +            # os.walk() lists them as directories.
> +            for i, entry in enumerate(dirs):
> +                if os.path.islink(os.path.join(rootpath, entry)):
> +                    del dirs[i]
> +                    files.append(entry)
> +
>              # Directory handling can happen in two ways, either DIRFILES is not set at all
>              # in which case we fall back to the older behaviour of packages owning all their
>              # directories
> 

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Christopher Larson]

[-- Attachment #1: Type: text/plain, Size: 1197 bytes --]

On Tue, Feb 17, 2015 at 9:54 AM, Mark Hatle <mark.hatle@windriver.com>
wrote:

> On 2/17/15 7:42 AM, Patrick Ohly wrote:
> > os.walk() returns symlinks to directories in the "dirs" lists,
> > but then never enters them by default. As a result, the old
> > code applied neither the directory handling (because that
> > is active once a directory gets entered) nor the file handling,
> > and thus never packaged such symlinks.
> >
> > The fix is simple: find such special directory entries and move
> > them to the "files" list.
>
> I'm curious, what is the problem that is being fixed with this.
>
> Normally you don't want to walk into symlinks to directories, as either
> you want
> to capture the symlink itself -- or the symlink is pointing to something
> you
> don't have access to when packaging (i.e. /proc).


This patch doesn't make it walk into the directory symlinks, it moves them
into the files list rather than the dirs list so they'll be captured
themselves, as far as I can tell from reading it.
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

[-- Attachment #2: Type: text/html, Size: 1703 bytes --]

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Mark Hatle]

On 2/17/15 10:57 AM, Christopher Larson wrote:
> 
> On Tue, Feb 17, 2015 at 9:54 AM, Mark Hatle <mark.hatle@windriver.com
> <mailto:mark.hatle@windriver.com>> wrote:
> 
>     On 2/17/15 7:42 AM, Patrick Ohly wrote:
>     > os.walk() returns symlinks to directories in the "dirs" lists,
>     > but then never enters them by default. As a result, the old
>     > code applied neither the directory handling (because that
>     > is active once a directory gets entered) nor the file handling,
>     > and thus never packaged such symlinks.
>     >
>     > The fix is simple: find such special directory entries and move
>     > them to the "files" list.
> 
>     I'm curious, what is the problem that is being fixed with this.
> 
>     Normally you don't want to walk into symlinks to directories, as either you want
>     to capture the symlink itself -- or the symlink is pointing to something you
>     don't have access to when packaging (i.e. /proc).
> 
> 
> This patch doesn't make it walk into the directory symlinks, it moves them into
> the files list rather than the dirs list so they'll be captured themselves, as
> far as I can tell from reading it.

Ok.. I missed that bit.. I'm still curious what this fixes.

--Mark

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Dan McGregor]

[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

On 17 Feb 2015 11:09, "Mark Hatle" <mark.hatle@windriver.com> wrote:
>
> On 2/17/15 10:57 AM, Christopher Larson wrote:
> >
> > On Tue, Feb 17, 2015 at 9:54 AM, Mark Hatle <mark.hatle@windriver.com
> > <mailto:mark.hatle@windriver.com>> wrote:
> >
> >     On 2/17/15 7:42 AM, Patrick Ohly wrote:
> >     > os.walk() returns symlinks to directories in the "dirs" lists,
> >     > but then never enters them by default. As a result, the old
> >     > code applied neither the directory handling (because that
> >     > is active once a directory gets entered) nor the file handling,
> >     > and thus never packaged such symlinks.
> >     >
> >     > The fix is simple: find such special directory entries and move
> >     > them to the "files" list.
> >
> >     I'm curious, what is the problem that is being fixed with this.
> >
> >     Normally you don't want to walk into symlinks to directories, as
either you want
> >     to capture the symlink itself -- or the symlink is pointing to
something you
> >     don't have access to when packaging (i.e. /proc).
> >
> >
> > This patch doesn't make it walk into the directory symlinks, it moves
them into
> > the files list rather than the dirs list so they'll be captured
themselves, as
> > far as I can tell from reading it.
>
> Ok.. I missed that bit.. I'm still curious what this fixes.
>

I haven't tried myself, but I expect it helps with the /usr merge.

> --Mark
>
> --
> _______________________________________________
> Openembedded-core mailing list
> Openembedded-core@lists.openembedded.org
> http://lists.openembedded.org/mailman/listinfo/openembedded-core

[-- Attachment #2: Type: text/html, Size: 2457 bytes --]

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

On Tue, 2015-02-17 at 21:40 -0600, Dan McGregor wrote:
> 
> On 17 Feb 2015 11:09, "Mark Hatle" <mark.hatle@windriver.com> wrote:
> >
> > On 2/17/15 10:57 AM, Christopher Larson wrote:
> > >
> > > On Tue, Feb 17, 2015 at 9:54 AM, Mark Hatle
> <mark.hatle@windriver.com
> > > <mailto:mark.hatle@windriver.com>> wrote:
> > > This patch doesn't make it walk into the directory symlinks, it moves them into
> > > the files list rather than the dirs list so they'll be captured themselves, as
> > > far as I can tell from reading it.
> >
> > Ok.. I missed that bit.. I'm still curious what this fixes.
> >
> 
> I haven't tried myself, but I expect it helps with the /usr merge.

Correct. I was working on a distro with a modified basefiles recipe
which only had /usr/[lib|bin|sbin] and symlinks to those in /. Without
this patch it was not possible to get these symlinks packaged in the
basefiles rpm.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

Re: [PATCH] package_rpm.bbclass: support packaging of symlinks to directories

[Mark Hatle]

On 2/18/15 2:45 AM, Patrick Ohly wrote:
> On Tue, 2015-02-17 at 21:40 -0600, Dan McGregor wrote:
>>
>> On 17 Feb 2015 11:09, "Mark Hatle" <mark.hatle@windriver.com> wrote:
>>>
>>> On 2/17/15 10:57 AM, Christopher Larson wrote:
>>>>
>>>> On Tue, Feb 17, 2015 at 9:54 AM, Mark Hatle
>> <mark.hatle@windriver.com
>>>> <mailto:mark.hatle@windriver.com>> wrote:
>>>> This patch doesn't make it walk into the directory symlinks, it moves them into
>>>> the files list rather than the dirs list so they'll be captured themselves, as
>>>> far as I can tell from reading it.
>>>
>>> Ok.. I missed that bit.. I'm still curious what this fixes.
>>>
>>
>> I haven't tried myself, but I expect it helps with the /usr merge.
> 
> Correct. I was working on a distro with a modified basefiles recipe
> which only had /usr/[lib|bin|sbin] and symlinks to those in /. Without
> this patch it was not possible to get these symlinks packaged in the
> basefiles rpm.
> 

Thanks, I wasn't aware that the symlinks were not working in the basefiles.

--Mark

[PATCH v2] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

os.walk() returns symlinks to directories in the "dirs" lists, but then never
enters them by default. As a result, the old code applied neither the
directory handling (because that is active once a directory gets entered) nor
the file handling, and thus never packaged such symlinks.

The fix is simple: find such special directory entries and move them to the
"files" list. However, one has to be careful about the undefined behavior of
modifying a list while iterating over it.

This fix was required for packaging a modified base-files that created
symlinks into /usr for /sbin /lib and /sbin.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/classes/package_rpm.bbclass | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index 4f9f813..e305e8b 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -199,10 +199,13 @@ python write_specfile () {
 
             # Treat all symlinks to directories as normal files.
             # os.walk() lists them as directories.
-            for i, entry in enumerate(dirs):
-                if os.path.islink(os.path.join(rootpath, entry)):
-                    del dirs[i]
-                    files.append(entry)
+            def move_to_files(dir):
+                if os.path.islink(os.path.join(rootpath, dir)):
+                    files.append(dir)
+                    return True
+                else:
+                    return False
+            dirs[:] = [dir for dir in dirs if not move_to_files(dir)]
 
             # Directory handling can happen in two ways, either DIRFILES is not set at all
             # in which case we fall back to the older behaviour of packages owning all their
-- 
1.8.4.5

Re: [PATCH v2] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

On Wed, 2015-02-25 at 06:51 -0800, Patrick Ohly wrote:
> diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
> index 4f9f813..e305e8b 100644
> --- a/meta/classes/package_rpm.bbclass
> +++ b/meta/classes/package_rpm.bbclass
> @@ -199,10 +199,13 @@ python write_specfile () {
>  
>              # Treat all symlinks to directories as normal files.
>              # os.walk() lists them as directories.
> -            for i, entry in enumerate(dirs):
> -                if os.path.islink(os.path.join(rootpath, entry)):
> -                    del dirs[i]
> -                    files.append(entry)
> +            def move_to_files(dir):
> +                if os.path.islink(os.path.join(rootpath, dir)):
> +                    files.append(dir)
> +                    return True
> +                else:
> +                    return False
> +            dirs[:] = [dir for dir in dirs if not move_to_files(dir)]
>  
>              # Directory handling can happen in two ways, either DIRFILES is not set at all
>              # in which case we fall back to the older behaviour of packages owning all their

Please ignore. I meant to squash the original patch and the list fix,
but was interrupted and ended up committing the fix separately with the
updated commit message. v3 is the patch that could be merged into
master.

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.

[PATCH v3] package_rpm.bbclass: support packaging of symlinks to directories

[Patrick Ohly]

os.walk() returns symlinks to directories in the "dirs" lists, but then never
enters them by default. As a result, the old code applied neither the
directory handling (because that is active once a directory gets entered) nor
the file handling, and thus never packaged such symlinks.

The fix is simple: find such special directory entries and move them to the
"files" list. However, one has to be careful about the undefined behavior of
modifying a list while iterating over it.

This fix was required for packaging a modified base-files that created
symlinks into /usr for /sbin /lib and /sbin.

Signed-off-by: Patrick Ohly <patrick.ohly@intel.com>
---
 meta/classes/package_rpm.bbclass | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/meta/classes/package_rpm.bbclass b/meta/classes/package_rpm.bbclass
index b87e634..e305e8b 100644
--- a/meta/classes/package_rpm.bbclass
+++ b/meta/classes/package_rpm.bbclass
@@ -197,6 +197,16 @@ python write_specfile () {
             if path.endswith("DEBIAN") or path.endswith("CONTROL"):
                 continue
 
+            # Treat all symlinks to directories as normal files.
+            # os.walk() lists them as directories.
+            def move_to_files(dir):
+                if os.path.islink(os.path.join(rootpath, dir)):
+                    files.append(dir)
+                    return True
+                else:
+                    return False
+            dirs[:] = [dir for dir in dirs if not move_to_files(dir)]
+
             # Directory handling can happen in two ways, either DIRFILES is not set at all
             # in which case we fall back to the older behaviour of packages owning all their
             # directories
-- 
1.8.4.5