[core-updates]: [PATCH 0/5]: Prevent wrap-progam from double-wrapping.

DoneSubmitted by Brendan Tildesley.
Details
3 participants
  • Danny Milosavljevic
  • Ludovic Courtès
  • Brendan Tildesley
Owner
unassigned
Severity
normal
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:39
(address . guix-patches@gnu.org)
83311dc4-6e9b-e70b-e379-9993bfcd0554@brendan.scot
I'm attempting to fix a bug where wrap-program produces ..X-real-real files by mistakenly wrapping already wrapped files. I haven't fully tested these because it requires rebuilding everything which takes hours to days and core-updates is stuck on mesa now anyway. Perhaps I'll try testing on master. Also there may be other places where .X-real files are accidentally wrapped, which will now error.
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:45
[PATCH 1/5] utils: wrap-program: Refuse to wrap .X-real files.
(address . 43367@debbugs.gnu.org)
20200913054557.12911-1-mail@brendan.scot
* guix/build/utils.scm: (wrap-program): Error if wrap-program wasmistakenly passed a .X-real file. This prevents and forces us to fixcases where a double wrapped ..X-real-real file is created, such as canbe seen with:
find /gnu/ -iname '.*-real-real'--- guix/build/utils.scm | 3 +++ 1 file changed, 3 insertions(+)
Toggle diff (16 lines)diff --git a/guix/build/utils.scm b/guix/build/utils.scmindex e872cfffd3..822191f4de 100644--- a/guix/build/utils.scm+++ b/guix/build/utils.scm@@ -1194,6 +1194,9 @@ with definitions for VARS." (format #f "export ~a=\"$~a${~a:+:}~a\"" var var var (string-join rest ":"))))) + (when (wrapped-program? prog)+ (error (string-append prog " is a wrapper. Refusing to wrap.")))+ (if already-wrapped? ;; PROG is already a wrapper: add the new "export VAR=VALUE" lines just-- 2.28.0
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:45
[PATCH 2/5] utils: Rename wrapper? to wrapped-program?.
(address . 43367@debbugs.gnu.org)
20200913054557.12911-2-mail@brendan.scot
* guix/build/utils.scm (wrap-program): The wrapper? procedure isincorrectly named as it actually checks to see if prog is theoriginal program that was moved, not the wrapper.
* guix/build/python-build-system: (wrap): Use renamed wrapped-program?.--- guix/build/python-build-system.scm | 2 +- guix/build/utils.scm | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-)
Toggle diff (39 lines)diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scmindex 62e7a7b305..d1dbbc1de2 100644--- a/guix/build/python-build-system.scm+++ b/guix/build/python-build-system.scm@@ -196,7 +196,7 @@ when running checks after installing the package." (define (list-of-files dir) (find-files dir (lambda (file stat) (and (eq? 'regular (stat:type stat))- (not (wrapper? file))))))+ (not (wrapped-program? file)))))) (define bindirs (append-map (match-lambdadiff --git a/guix/build/utils.scm b/guix/build/utils.scmindex 822191f4de..4cd227a668 100644--- a/guix/build/utils.scm+++ b/guix/build/utils.scm@@ -90,7 +90,7 @@ patch-/usr/bin/file fold-port-matches remove-store-references- wrapper?+ wrapped-program? wrap-program wrap-script @@ -1118,8 +1118,8 @@ known as `nuke-refs' in Nixpkgs." (program wrap-error-program) (type wrap-error-type)) -(define (wrapper? prog)- "Return #t if PROG is a wrapper as produced by 'wrap-program'."+(define (wrapped-program? prog)+ "Return #t if PROG is a program that was moved and wrapped by 'wrap-program'." (and (file-exists? prog) (let ((base (basename prog))) (and (string-prefix? "." base)-- 2.28.0
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:45
[PATCH 3/5] glib-or-gtk-build-system: Don't double wrap programs.
(address . 43367@debbugs.gnu.org)
20200913054557.12911-3-mail@brendan.scot
* guix/build/glib-or-gtk-build-system.scm (wrap-all-programs): If apackage definition was modified to insert an additional wrap phasebefore glib-or-gtk...'s wrap phase instead of after, glib-or-gtk...'swrap phase will double wrap the .X-real file from the earlier wrapphase. Filtering out such wrapped programs means these .X-real filesshould fix this and mean packagers don't have to worry about ensuringtheir wrap phases are put afterwards.--- guix/build/glib-or-gtk-build-system.scm | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
Toggle diff (18 lines)diff --git a/guix/build/glib-or-gtk-build-system.scm b/guix/build/glib-or-gtk-build-system.scmindex ba680fd1a9..ccb3138fe2 100644--- a/guix/build/glib-or-gtk-build-system.scm+++ b/guix/build/glib-or-gtk-build-system.scm@@ -142,8 +142,9 @@ add a dependency of that output on GLib and GTK+." (unless (member output glib-or-gtk-wrap-excluded-outputs) (let* ((bindir (string-append directory "/bin")) (libexecdir (string-append directory "/libexec"))- (bin-list (append (find-files bindir ".*")- (find-files libexecdir ".*")))+ (bin-list (filter (negate wrapped-program?)+ (append (find-files bindir ".*")+ (find-files libexecdir ".*")))) (datadirs (data-directories (alist-cons output directory inputs))) (gtk-mod-dirs (gtk-module-directories-- 2.28.0
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:45
[PATCH 4/5] rakudo-build-system: Don't double wrap programs.
(address . 43367@debbugs.gnu.org)
20200913054557.12911-4-mail@brendan.scot
* guix/build/rakudo-build-system.scm (wrap): Don't return any potentialalready wrapped-programs in the list-of-files to wrap.--- guix/build/rakudo-build-system.scm | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
Toggle diff (16 lines)diff --git a/guix/build/rakudo-build-system.scm b/guix/build/rakudo-build-system.scmindex dbdeb1ccd2..b2c090f946 100644--- a/guix/build/rakudo-build-system.scm+++ b/guix/build/rakudo-build-system.scm@@ -97,7 +97,8 @@ (map (cut string-append dir "/" <>) (or (scandir dir (lambda (f) (let ((s (stat (string-append dir "/" f))))- (eq? 'regular (stat:type s)))))+ (and (eq? 'regular (stat:type s))+ (not (wrapped-program? f)))))) '()))) (define bindirs-- 2.28.0
B
B
Brendan Tildesley wrote on 13 Sep 2020 07:45
[PATCH 5/5] qt-build-system: Don't double wrap programs.
(address . 43367@debbugs.gnu.org)
20200913054557.12911-5-mail@brendan.scot
* guix/build/qt-build-system.scm (wrap-all-programs): Excluded wrappedprograms from the list of files to wrap if they exist to avoid doublewrapping.--- guix/build/qt-build-system.scm | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Toggle diff (18 lines)diff --git a/guix/build/qt-build-system.scm b/guix/build/qt-build-system.scmindex 005157b0a4..4738ca09c9 100644--- a/guix/build/qt-build-system.scm+++ b/guix/build/qt-build-system.scm@@ -83,7 +83,10 @@ add a dependency of that output on Qt." (define (find-files-to-wrap directory) (append-map (lambda (dir)- (if (directory-exists? dir) (find-files dir ".*") (list)))+ (if (directory-exists? dir)+ (find-files dir (lambda (file stat)+ (not (wrapped-program? file))))+ '())) (list (string-append directory "/bin") (string-append directory "/sbin") (string-append directory "/libexec")-- 2.28.0
D
D
Danny Milosavljevic wrote on 13 Sep 2020 11:40
Re: [bug#43367] [core-updates]: [PATCH 0/5]: Prevent wrap-progam from double-wrapping.
(name . Brendan Tildesley)(address . mail@brendan.scot)(address . 43367@debbugs.gnu.org)
20200913114019.58a6bda0@scratchpost.org
On Sun, 13 Sep 2020 15:39:15 +1000Brendan Tildesley <mail@brendan.scot> wrote:
Toggle quote (7 lines)> I'm attempting to fix a bug where wrap-program produces ..X-real-real > files by mistakenly wrapping already wrapped files. I haven't fully > tested these because it requires rebuilding everything which takes hours > to days and core-updates is stuck on mesa now anyway. Perhaps I'll try > testing on master. Also there may be other places where .X-real files > are accidentally wrapped, which will now error.
But can't a thing be wrapped once for one reason and another time for anotherreason and that should be fine?
-----BEGIN PGP SIGNATURE-----
iQEzBAEBCgAdFiEEds7GsXJ0tGXALbPZ5xo1VCwwuqUFAl9d6QMACgkQ5xo1VCwwuqXYBAf/SYsAR5A4AYq/DdVDnV1V2ZqmOEBgR2bBwvk65kl2ZFJuFr5AzXvhw59ODVVVhDUtx4I+yk8oq3o/ySovtnOUDEwIaukAsxWZekQt2/+6fAE3iFI3lVZCvO5xLQdpIIGJpqIBPgPyZqaxF/pMM7MpVkKi3MmHYk1+QoAgYKhv/Hz1lUdZ/BPoDs5Z+EoJq8vOsmm9c5zLn+SXO5R5bPEvWrItRxVeWXHaulbR/T5j8hJLBSLoR3do55og47tdFZDZ8wrk+KfLrUQ358XoycB+vcfhE7204XRzd9i4zjY4taoggnGg+Y13WDS5gpbrAvic3KNROJ/QfmADkVgkkukjfA===G/xm-----END PGP SIGNATURE-----

B
B
Brendan Tildesley wrote on 13 Sep 2020 14:30
(name . Danny Milosavljevic)(address . dannym@scratchpost.org)(address . 43367@debbugs.gnu.org)
6c55ad27-6661-c84f-53ec-0baaa9c9ce91@brendan.scot
On 13/9/20 7:40 pm, Danny Milosavljevic wrote:
Toggle quote (12 lines)> On Sun, 13 Sep 2020 15:39:15 +1000> Brendan Tildesley <mail@brendan.scot> wrote:>>> I'm attempting to fix a bug where wrap-program produces ..X-real-real>> files by mistakenly wrapping already wrapped files. I haven't fully>> tested these because it requires rebuilding everything which takes hours>> to days and core-updates is stuck on mesa now anyway. Perhaps I'll try>> testing on master. Also there may be other places where .X-real files>> are accidentally wrapped, which will now error.> But can't a thing be wrapped once for one reason and another time for another> reason and that should be fine?
Yes, perhaps I should have explained that this is still possible and works fine. When a program is wrapped a second time, it will append to the existed wrapper, rather than creating a new file and moving the old one. repeated applications of wrap-program after the first one simply append. I'll illustrate how this can go wrong though: suppose we have /bin/foo and we we are in a repl and run:
(wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real doesn't exist so /bin/foo is moved to /bin/.foo-real, a new /bin/foo is created that is a wrapper that then launches /bin.foo-real.
(wrap-program "/bin/foo" `("BAR" = ("baz")) => /bin/.foo-real exists so /bin/foo is assumed to already be a wrapper so variables are appended to /bin/foo.
(wrap-program "/bin/foo" `("BAR" = ("baz")) => same thing again, variables are appended
; Now suppose we then run:
(wrap-program "/bin/.foo-real" `("BAR" = ("baz")) => /bin/..foo-real-real doesn't exist, so /bin/.foo-real is moved to /bin/..foo-real-real and /bin/.foo-real is created again as another wrapper.
This should never be done intentionally I think, but sometimes there is code that uses (find-files dir ".") to find binaries to wrap, and this is run after a previous existing wrap phase, so the both /bin/foo and /bin/.foo-real are wrapped again. Generally everything will continue working though despite all this though.
You run this to find some of these double wrapped packages:
find /gnu/ -maxdepth 4 -iname '.*-real-real'
So I thought it best to error whenever this happens instead of allowing it.
An example of this causing an issue is when Prafulla Giri posted a patch[0] to fix a bug with Calibre. Their code ought to be correct, but it resulted in double wrapping. I created my own patch by overwriting the python-build-systems wrap phase and duplicating some code. Andreas ended up accepting my patch instead.
... Actually I just realised Prafulla's patch could have been fixed in a much simpler way by adjusting the (find-files ...) bit and avoided duplication. ...
Anyway, with these patches, Prafulla's patch would have caused an error and forced them to fix it, for example, by changing
(find-files "." ".")
to
(find-files "." (lambda (file stat) (not (wrapper? file))))or(find-files "." (lambda (file stat) (not (string-prefix "." (basename file))))
----------
So, the main change here is making (wrap-program ".foo-real") an error. If you cannot think of a good reason why that should ever be run, I think its good to block it. bugs that can slip through easily and lurk in the background usually not causing problems are not good in my opinion. After that has been decided we need to ensure all build systems don't misuse wrap-program that way. I notice some build systems actually only pass 'regular files, others allow symlinks or any file. I'm not really sure what the exact find-files filter should be.
[0] https://lists.gnu.org/archive/html/guix-patches/2020-09/msg00219.html
Attachment: file
L
L
Ludovic Courtès wrote on 22 Apr 11:06 +0200
Re: bug#43367: [core-updates]: [PATCH 0/5]: Prevent wrap-progam from double-wrapping.
(name . Brendan Tildesley)(address . mail@brendan.scot)(address . 43367-done@debbugs.gnu.org)
87im4e1yfa.fsf@gnu.org
Hi Brendan,
Brendan Tildesley <mail@brendan.scot> skribis:
Toggle quote (7 lines)> I'm attempting to fix a bug where wrap-program produces ..X-real-real> files by mistakenly wrapping already wrapped files. I haven't fully > tested these because it requires rebuilding everything which takes> hours to days and core-updates is stuck on mesa now anyway. Perhaps> I'll try testing on master. Also there may be other places where> .X-real files are accidentally wrapped, which will now error.
The patch series LGTM and I’ve applied it on ‘core-updates’. I’mbuilding things now and will push shortly if everything goes well.
Thank you and sorry for the loooong delay!
Ludo’.
Closed
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send email to 43367@debbugs.gnu.org