diff mbox series

deb-dl-dir class rework to use faster ln -P or fallback to cp

Message ID 20230206130919.327653-1-roberto.foglietta@linuxteam.org
State Superseded, archived
Headers show
Series deb-dl-dir class rework to use faster ln -P or fallback to cp | expand

Commit Message

roberto.foglietta@linuxteam.org Feb. 6, 2023, 1:09 p.m. UTC 
From: "Roberto A. Foglietta" <roberto.foglietta@gmail.com>

deb-dl-dir, feature: faster when using ln -P otherwise fallback to cp

The original class functions deb_dl_dir_import/export were using cp to
copy debian package to the target rootfs but this approach is quite slow
while using hard link does not work if the destination and source dirs
are not lying on the same filesystem. Thus, ln -P should fallback to cp
when it does not work (which is different from complaining on stderr).

Moreover, these two functions have been reworked to reach a straight
forward and more compact form. In particular, export function was using
bashism to do some kind of comparison which after all is useless
because copying back without overwriting just fulfills that part.

Signed-off-by: Roberto A. Foglietta <roberto.foglietta@gmail.com>
---
 meta/classes/deb-dl-dir.bbclass | 41 ++++++++++++++++++---------------
 1 file changed, 23 insertions(+), 18 deletions(-)

Comments

Uladzimir Bely Feb. 7, 2023, 6:32 a.m. UTC | #1 
In the email from понедельник, 6 февраля 2023 г. 16:09:19 +03 user 
roberto.foglietta@linuxteam.org wrote:
> From: "Roberto A. Foglietta" <roberto.foglietta@gmail.com>
> 
> deb-dl-dir, feature: faster when using ln -P otherwise fallback to cp
> 
> The original class functions deb_dl_dir_import/export were using cp to
> copy debian package to the target rootfs but this approach is quite slow
> while using hard link does not work if the destination and source dirs
> are not lying on the same filesystem. Thus, ln -P should fallback to cp
> when it does not work (which is different from complaining on stderr).
> 
> Moreover, these two functions have been reworked to reach a straight
> forward and more compact form. In particular, export function was using
> bashism to do some kind of comparison which after all is useless
> because copying back without overwriting just fulfills that part.
> 
> Signed-off-by: Roberto A. Foglietta <roberto.foglietta@gmail.com>
> ---
>  meta/classes/deb-dl-dir.bbclass | 41 ++++++++++++++++++---------------
>  1 file changed, 23 insertions(+), 18 deletions(-)
> 
> diff --git a/meta/classes/deb-dl-dir.bbclass
> b/meta/classes/deb-dl-dir.bbclass index 3b1517dc..22de7bf6 100644
> --- a/meta/classes/deb-dl-dir.bbclass
> +++ b/meta/classes/deb-dl-dir.bbclass
> @@ -78,39 +78,44 @@ debsrc_download() {
> 
>  deb_dl_dir_import() {
>      export pc="${DEBDIR}/${2}"
> -    export rootfs="${1}"
> -    sudo mkdir -p "${rootfs}"/var/cache/apt/archives/
> +    export sc="${1}/var/cache/apt/archives/"
> +    sudo mkdir -p "${sc}"
>      [ ! -d "${pc}" ] && return 0
> +    export tf=$(ls -1 "${pc}"/*.deb | head -n1)
> +    [ ! -e "${tf}" ] && return 0
>      flock -s "${pc}".lock -c '
>          set -e
>          printenv | grep -q BB_VERBOSE_LOGS && set -x
> 
> -        sudo find "${pc}" -type f -iname "*\.deb" -exec \
> -            cp -n --no-preserve=owner -t
> "${rootfs}"/var/cache/apt/archives/ {} + +        sudo ln -Pf -t "${sc}"
> "${tf}" 2>/dev/null ||:

I'm not sure that assumption "if one hardlink failed, other also will fail" 
(and vice versa) would always be true.

The solution in "[PATCH v2] deb-dl-dir: Fallback to copying when import-
export" looks simpler and works in per-file basis.

> +        if [ -r "${tf}" ]; then
> +            sudo find "${pc}" -type f -iname "*\.deb" -exec \
> +                ln -Pf -t "${sc}" {} +
> +        else
> +            sudo find "${pc}" -type f -iname "*\.deb" -exec \
> +                cp -np owner --reflink=auto -t "${sc}" {} +
> +        fi
>      '
>  }
> 
>  deb_dl_dir_export() {
>      export pc="${DEBDIR}/${2}"
> -    export rootfs="${1}"
> +    export sc="${1}/var/cache/apt/archives/"
>      mkdir -p "${pc}"
> +    export tf=$(ls -1 "${sc}"/*.deb | head -n1)
> +    [ ! -e "${tf}" ] && return 0
>      flock "${pc}".lock -c '
>          set -e
>          printenv | grep -q BB_VERBOSE_LOGS && set -x
> 
> -        find "${rootfs}"/var/cache/apt/archives/ \
> -            -maxdepth 1 -type f -iname '*\.deb' |\
> -        while read p; do
> -            # skip files from a previous export
> -            [ -f "${pc}/${p##*/}" ] && continue
> -            # can not reuse bitbake function here, this is basically
> -            # "repo_contains_package"
> -            package=$(find "${REPO_ISAR_DIR}"/"${DISTRO}" -name ${p##*/})
> -            if [ -n "$package" ]; then
> -                cmp --silent "$package" "$p" && continue
> -            fi
> -            sudo cp -n "${p}" "${pc}"
> -        done
> +        sudo ln -Pf -t "${pc}" "${tf}" 2>/dev/null ||:
> +        if [ -r "${tf}" ]; then
> +            sudo find "${sc}" -maxdepth 1 -type f -iname '*\.deb' \
> +                -exec ln -P -t "${pc}" {} + 2>/dev/null ||:
> +        else
> +            sudo find "${sc}" -maxdepth 1 -type f -iname '*\.deb' \
> +                -exec cp -n --reflink=auto -t "${pc}" {} +
> +        fi
>          sudo chown -R $(id -u):$(id -g) "${pc}"
>      '
>  }
Roberto A. Foglietta Feb. 7, 2023, 7:37 a.m. UTC | #2 
On Tue, 7 Feb 2023 at 07:32, Uladzimir Bely <ubely@ilbers.de> wrote:
>
> In the email from понедельник, 6 февраля 2023 г. 16:09:19 +03 user
> roberto.foglietta@linuxteam.org wrote:
> > From: "Roberto A. Foglietta" <roberto.foglietta@gmail.com>
> >
> > deb-dl-dir, feature: faster when using ln -P otherwise fallback to cp
> >
> > The original class functions deb_dl_dir_import/export were using cp to
> > copy debian package to the target rootfs but this approach is quite slow
> > while using hard link does not work if the destination and source dirs
> > are not lying on the same filesystem. Thus, ln -P should fallback to cp
> > when it does not work (which is different from complaining on stderr).
> >
> > Moreover, these two functions have been reworked to reach a straight
> > forward and more compact form. In particular, export function was using
> > bashism to do some kind of comparison which after all is useless
> > because copying back without overwriting just fulfills that part.
> >
> > Signed-off-by: Roberto A. Foglietta <roberto.foglietta@gmail.com>
> > ---
> >  meta/classes/deb-dl-dir.bbclass | 41 ++++++++++++++++++---------------
> >  1 file changed, 23 insertions(+), 18 deletions(-)
> >
> > diff --git a/meta/classes/deb-dl-dir.bbclass
> > b/meta/classes/deb-dl-dir.bbclass index 3b1517dc..22de7bf6 100644
> > --- a/meta/classes/deb-dl-dir.bbclass
> > +++ b/meta/classes/deb-dl-dir.bbclass
> > @@ -78,39 +78,44 @@ debsrc_download() {
> >
> >  deb_dl_dir_import() {
> >      export pc="${DEBDIR}/${2}"
> > -    export rootfs="${1}"
> > -    sudo mkdir -p "${rootfs}"/var/cache/apt/archives/
> > +    export sc="${1}/var/cache/apt/archives/"
> > +    sudo mkdir -p "${sc}"
> >      [ ! -d "${pc}" ] && return 0
> > +    export tf=$(ls -1 "${pc}"/*.deb | head -n1)
> > +    [ ! -e "${tf}" ] && return 0
> >      flock -s "${pc}".lock -c '
> >          set -e
> >          printenv | grep -q BB_VERBOSE_LOGS && set -x
> >
> > -        sudo find "${pc}" -type f -iname "*\.deb" -exec \
> > -            cp -n --no-preserve=owner -t
> > "${rootfs}"/var/cache/apt/archives/ {} + +        sudo ln -Pf -t "${sc}"
> > "${tf}" 2>/dev/null ||:
>
> I'm not sure that assumption "if one hardlink failed, other also will fail"
> (and vice versa) would always be true.

Yes, it is true. If further failures happen it is not because of the
destination/source nature. Instead of the opposite, the return code
does not always lead to a failure in a way we suppose.

Best regards, R-
diff mbox series

Patch

diff --git a/meta/classes/deb-dl-dir.bbclass b/meta/classes/deb-dl-dir.bbclass
index 3b1517dc..22de7bf6 100644
--- a/meta/classes/deb-dl-dir.bbclass
+++ b/meta/classes/deb-dl-dir.bbclass
@@ -78,39 +78,44 @@  debsrc_download() {
 
 deb_dl_dir_import() {
     export pc="${DEBDIR}/${2}"
-    export rootfs="${1}"
-    sudo mkdir -p "${rootfs}"/var/cache/apt/archives/
+    export sc="${1}/var/cache/apt/archives/"
+    sudo mkdir -p "${sc}"
     [ ! -d "${pc}" ] && return 0
+    export tf=$(ls -1 "${pc}"/*.deb | head -n1)
+    [ ! -e "${tf}" ] && return 0
     flock -s "${pc}".lock -c '
         set -e
         printenv | grep -q BB_VERBOSE_LOGS && set -x
 
-        sudo find "${pc}" -type f -iname "*\.deb" -exec \
-            cp -n --no-preserve=owner -t "${rootfs}"/var/cache/apt/archives/ {} +
+        sudo ln -Pf -t "${sc}" "${tf}" 2>/dev/null ||:
+        if [ -r "${tf}" ]; then
+            sudo find "${pc}" -type f -iname "*\.deb" -exec \
+                ln -Pf -t "${sc}" {} +
+        else
+            sudo find "${pc}" -type f -iname "*\.deb" -exec \
+                cp -np owner --reflink=auto -t "${sc}" {} +
+        fi
     '
 }
 
 deb_dl_dir_export() {
     export pc="${DEBDIR}/${2}"
-    export rootfs="${1}"
+    export sc="${1}/var/cache/apt/archives/"
     mkdir -p "${pc}"
+    export tf=$(ls -1 "${sc}"/*.deb | head -n1)
+    [ ! -e "${tf}" ] && return 0
     flock "${pc}".lock -c '
         set -e
         printenv | grep -q BB_VERBOSE_LOGS && set -x
 
-        find "${rootfs}"/var/cache/apt/archives/ \
-            -maxdepth 1 -type f -iname '*\.deb' |\
-        while read p; do
-            # skip files from a previous export
-            [ -f "${pc}/${p##*/}" ] && continue
-            # can not reuse bitbake function here, this is basically
-            # "repo_contains_package"
-            package=$(find "${REPO_ISAR_DIR}"/"${DISTRO}" -name ${p##*/})
-            if [ -n "$package" ]; then
-                cmp --silent "$package" "$p" && continue
-            fi
-            sudo cp -n "${p}" "${pc}"
-        done
+        sudo ln -Pf -t "${pc}" "${tf}" 2>/dev/null ||:
+        if [ -r "${tf}" ]; then
+            sudo find "${sc}" -maxdepth 1 -type f -iname '*\.deb' \
+                -exec ln -P -t "${pc}" {} + 2>/dev/null ||:
+        else
+            sudo find "${sc}" -maxdepth 1 -type f -iname '*\.deb' \
+                -exec cp -n --reflink=auto -t "${pc}" {} +
+        fi
         sudo chown -R $(id -u):$(id -g) "${pc}"
     '
 }