[fetcher2] perforce fetcher download dir

[fetcher2] perforce fetcher download dir

[Alexandru N. Onea]

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

Hello community,

I just started using the perforce fetcher and I am a bit confused by how
the fetcher implementation handles the location of the downloaded sources.
In particular, in cases where SRC_URI points to a directory, expressed by
the typical "/..." termination, the below code seems to prepare the final
filename by only taking into account the part of the remote path which
would follow the "/...".

if depot_filename:
    if ud.pathisdir: # Remove leading path to obtain filename
        filename = depot_filename[len(ud.path)-1:]
    else:
        filename = depot_filename[depot_filename.rfind('/'):]
    filename = filename[:filename.find('#')] # Remove trailing '#rev'

This comes with a limitation: if one has multiple perforce SRC_URI entries,
one ends up with a mixture of files, not necessarily belonging together.
Moreover, the comment is a bit misleading, since the value of filename
after the removal of the leading path is not actually the filename but a
filepath, stripped of the "visible" repo path given in SRC_URI.

Of course, there might be a strong argument in favor of this behavior,
however I failed so far to find it. I am writing to you in the hope that
someone can help me find the argument and understand the rationale of the
current implementation.

If there is no argument in favor of the mentioned behavior, I would like to
prepare a patch for an alternative where the user has the option to either
keep the full remote file path (similar to how a perforce client would sync
the files) or strip the visible SRC_URI part of the path, but specify a
subdir name for each perforce SRC_URI entry in order to keep things
separated.

Thank you,
Alexandru Onea

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

Re: [bitbake-devel] [fetcher2] perforce fetcher download dir

[Richard Purdie]

Hi,

On Sat, 2020-06-20 at 15:33 +0300, Alexandru N. Onea wrote:
> I just started using the perforce fetcher and I am a bit confused by
> how the fetcher implementation handles the location of the downloaded
> sources. In particular, in cases where SRC_URI points to a directory,
> expressed by the typical "/..." termination, the below code seems to
> prepare the final filename by only taking into account the part of
> the remote path which would follow the "/...".
> 
> if depot_filename:
>     if ud.pathisdir: # Remove leading path to obtain filename
>         filename = depot_filename[len(ud.path)-1:]
>     else:
>         filename = depot_filename[depot_filename.rfind('/'):]
>     filename = filename[:filename.find('#')] # Remove trailing '#rev'
> 
> This comes with a limitation: if one has multiple perforce SRC_URI
> entries, one ends up with a mixture of files, not
> necessarily belonging together. Moreover, the comment is a bit
> misleading, since the value of filename after the removal of the
> leading path is not actually the filename but a filepath, stripped of
> the "visible" repo path given in SRC_URI.
> 
> Of course, there might be a strong argument in favor of this
> behavior, however I failed so far to find it. I am writing to you in
> the hope that someone can help me find the argument and understand
> the rationale of the current implementation.
> 
> If there is no argument in favor of the mentioned behavior, I would
> like to prepare a patch for an alternative where the user has the
> option to either keep the full remote file path (similar to how a
> perforce client would sync the files) or strip the visible SRC_URI
> part of the path, but specify a subdir name for each perforce SRC_URI
> entry in order to keep things separated. 

Unfortunately the perforce fetcher is a bit of a black box to me and
most other bitbake developers, we don't have the tool or any repos to
use/test with it.

We're therefore reliant on users reporting and fixing issues they run
into.

Thanks for your patches I saw yesterday, at a first glance they look
reasonable. My only real concern is whether they overlap with any of
our unpack options. I also wondered if we should be updating the
bitbake manual with any of these new options or more information about
the perforce fetcher?

Cheers,

Richard

Re: [bitbake-devel] [fetcher2] perforce fetcher download dir

[Alexandru N. Onea]

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

Hi,

Indeed, there is an overlap between the "destdir" option introduced by the
patch and the "subdir" option of the unpack. Their behavior is only
slightly different but the subdir option can be used to achieve the same
result. I will drop the "destdir" option. The other options do not have any
overlap with existing unpack functionality.

I will prepare a new version of the patch including updated documentation.

Best regards,
Alexandru Onea


On Mon, 22 Jun 2020 at 23:18, Richard Purdie <
richard.purdie@linuxfoundation.org> wrote:

> Hi,
>
> On Sat, 2020-06-20 at 15:33 +0300, Alexandru N. Onea wrote:
> > I just started using the perforce fetcher and I am a bit confused by
> > how the fetcher implementation handles the location of the downloaded
> > sources. In particular, in cases where SRC_URI points to a directory,
> > expressed by the typical "/..." termination, the below code seems to
> > prepare the final filename by only taking into account the part of
> > the remote path which would follow the "/...".
> >
> > if depot_filename:
> >     if ud.pathisdir: # Remove leading path to obtain filename
> >         filename = depot_filename[len(ud.path)-1:]
> >     else:
> >         filename = depot_filename[depot_filename.rfind('/'):]
> >     filename = filename[:filename.find('#')] # Remove trailing '#rev'
> >
> > This comes with a limitation: if one has multiple perforce SRC_URI
> > entries, one ends up with a mixture of files, not
> > necessarily belonging together. Moreover, the comment is a bit
> > misleading, since the value of filename after the removal of the
> > leading path is not actually the filename but a filepath, stripped of
> > the "visible" repo path given in SRC_URI.
> >
> > Of course, there might be a strong argument in favor of this
> > behavior, however I failed so far to find it. I am writing to you in
> > the hope that someone can help me find the argument and understand
> > the rationale of the current implementation.
> >
> > If there is no argument in favor of the mentioned behavior, I would
> > like to prepare a patch for an alternative where the user has the
> > option to either keep the full remote file path (similar to how a
> > perforce client would sync the files) or strip the visible SRC_URI
> > part of the path, but specify a subdir name for each perforce SRC_URI
> > entry in order to keep things separated.
>
> Unfortunately the perforce fetcher is a bit of a black box to me and
> most other bitbake developers, we don't have the tool or any repos to
> use/test with it.
>
> We're therefore reliant on users reporting and fixing issues they run
> into.
>
> Thanks for your patches I saw yesterday, at a first glance they look
> reasonable. My only real concern is whether they overlap with any of
> our unpack options. I also wondered if we should be updating the
> bitbake manual with any of these new options or more information about
> the perforce fetcher?
>
> Cheers,
>
> Richard
>
>

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

Re: [bitbake-devel] [fetcher2] perforce fetcher download dir

[Richard Purdie]

On Tue, 2020-06-23 at 09:40 +0300, Alexandru Onea wrote:
> Indeed, there is an overlap between the "destdir" option introduced
> by the patch and the "subdir" option of the unpack. Their behavior is
> only slightly different but the subdir option can be used to achieve
> the same result. I will drop the "destdir" option. The other options
> do not have any overlap with existing unpack functionality.

That was what I suspected and its good to have only one API for doing
things where we can!

> I will prepare a new version of the patch including
> updated documentation.

The new patches look good, I've queued for testing, thanks.

Cheers,

Richard