Daemon does not honor “useSubstitutes” on armhf

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Timothy Sample
Owner
unassigned
Submitted by
Timothy Sample
Severity
normal
T
T
Timothy Sample wrote on 11 Sep 2019 20:51
(address . bug-guix@gnu.org)
87woeevf8k.fsf@ngyro.com
Currently, the “guix” package cannot build on armhf due to test
confirm this on my local machine, too.)

It looks like there is some confusion about the layout of the “Settings”
struct. At least, if I add some print statements in the code to print
the address of the “useSubstitutes” flag:

printMsg(lvlError, format("XXX: %1%") % &settings.useSubstitutes);

it gives me different addresses depending on whether I’m printing from
the main daemon loop or from the “querySubstitutablePathInfos” method.
The addresses suspiciously differ by eight.

I can confirm that the addresses are the same on x86_64.

My guess is that this bug was revealed by the changes made in commit
f6919ebdc6b0ce0286814cc6ab0564b1a4c67f5f (making the daemon assume a
single substituter). Before this commit, it looks like the daemon would
give up because there were no substituters. Now it relies on checking
“useSubstitutes”, which is not in fact reliable. I’ve tested everything
from before this commit, and the addresses are still different.

Weirdly, in GDB, I can be right before the following line:

if (!settings.useSubstitutes) return;

run “p settings.useSubstitutes” and see “false”, and then continue
running through the method without hitting that “return”. It seems GDB
is not confused about the flag, just the actual code.

Looking at the disassembly was no use to me, since I don’t know much
about ARM, and it was not simple enough for me to guess what it was
doing.


-- Tim
T
T
Timothy Sample wrote on 11 Sep 2019 22:28
Re: bug#37384: Daemon does not honor “useSubsti tutes” on armhf
(address . 37384@debbugs.gnu.org)
87sgp2vaqh.fsf@ngyro.com
Timothy Sample <samplet@ngyro.com> writes:

Toggle quote (4 lines)
> it gives me different addresses depending on whether I’m printing from
> the main daemon loop or from the “querySubstitutablePathInfos” method.
> The addresses suspiciously differ by eight.

The problem goes away if I move “useSubstitutes” above “reservedSize”,
which has type “off_t”. I’m guessing this is because of something fishy
with “_FILE_OFFSET_BITS”, but I’m not sure what just yet.


-- Tim
T
T
Timothy Sample wrote on 12 Sep 2019 07:47
(address . 37384@debbugs.gnu.org)
87ftl25am0.fsf@ngyro.com
Timothy Sample <samplet@ngyro.com> writes:

Toggle quote (4 lines)
> The problem goes away if I move “useSubstitutes” above “reservedSize”,
> which has type “off_t”. I’m guessing this is because of something fishy
> with “_FILE_OFFSET_BITS”, but I’m not sure what just yet.

Here’s my best guess as to what is going on. In “nix-daemon.cc” the
order of includes causes “off_t” to be defined before
“_FILE_OFFSET_BITS”, which results in it being something like a 32-bit
signed integer. In “local-store.cc”, “_FILE_OFFSET_BITS” gets set
first, yielding a 64-bit “off_t”. This causes all of the addresses in
the “Settings” struct to be mismatched after “reservedSize”. I plugged
in “sizeof(off_t)” into the “printMsg” calls from before, and (lo and
behold) I saw 4 from “nix-daemon.cc” and 8 from “local-store.cc”.

The following patch fixes the problem:
From 3c3eafac82e0a6e8a37363d6eb46f128e585705a Mon Sep 17 00:00:00 2001
From: Timothy Sample <samplet@ngyro.com>
Date: Thu, 12 Sep 2019 00:50:54 -0400
Subject: [PATCH] daemon: Include 'config.h' in 'nix-daemon.cc'.

* nix/nix-daemon/nix-daemon.cc: Include 'config.h'.
---
nix/nix-daemon/nix-daemon.cc | 1 +
1 file changed, 1 insertion(+)

Toggle diff (11 lines)
diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
index ffac6cde34..1163a249d1 100644
--- a/nix/nix-daemon/nix-daemon.cc
+++ b/nix/nix-daemon/nix-daemon.cc
@@ -1,3 +1,4 @@
+#include "config.h"
#include "shared.hh"
#include "local-store.hh"
#include "util.hh"
--
2.23.0
Is this okay to push? It seems kind of conspicuous that it’s missing in
the first place. Is it missing for a good reason?


-- Tim
L
L
Ludovic Courtès wrote on 16 Sep 2019 17:52
(name . Timothy Sample)(address . samplet@ngyro.com)(address . 37384@debbugs.gnu.org)
877e686xwk.fsf@gnu.org
Hello Timothy,

Timothy Sample <samplet@ngyro.com> skribis:

Toggle quote (22 lines)
> The following patch fixes the problem:
>
> From 3c3eafac82e0a6e8a37363d6eb46f128e585705a Mon Sep 17 00:00:00 2001
> From: Timothy Sample <samplet@ngyro.com>
> Date: Thu, 12 Sep 2019 00:50:54 -0400
> Subject: [PATCH] daemon: Include 'config.h' in 'nix-daemon.cc'.
>
> * nix/nix-daemon/nix-daemon.cc: Include 'config.h'.
> ---
> nix/nix-daemon/nix-daemon.cc | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/nix/nix-daemon/nix-daemon.cc b/nix/nix-daemon/nix-daemon.cc
> index ffac6cde34..1163a249d1 100644
> --- a/nix/nix-daemon/nix-daemon.cc
> +++ b/nix/nix-daemon/nix-daemon.cc
> @@ -1,3 +1,4 @@
> +#include "config.h"
> #include "shared.hh"
> #include "local-store.hh"
> #include "util.hh"

LGTM.

Thanks for the debugging effort that led to this fix!

Ludo’.
L
L
Ludovic Courtès wrote on 16 Sep 2019 23:06
Re: bug#37426: guix-1.0.1-5.cc98b00 fails tests on i686-linux
(name . Mikhail Kryshen)(address . mikhail@kryshen.net)
871rwg2bp5.fsf@gnu.org
Hi Mikhail,

Mikhail Kryshen <mikhail@kryshen.net> skribis:

Toggle quote (4 lines)
>
> This breaks system configuration on i686 with current Guix.

Thanks for the heads-up.

The test failures are:

Toggle snippet (77 lines)
test-name: substitutable-path-info when substitutes are turned off
location: /tmp/guix-build-guix-1.0.1-5.cc98b00.drv-0/source/tests/store.scm:310
source:
+ (test-equal
+ "substitutable-path-info when substitutes are turned off"
+ '()
+ (with-store
+ s
+ (set-build-options s #:use-substitutes? #f)
+ (let* ((b (add-to-store
+ s
+ "bash"
+ #t
+ "sha256"
+ (search-bootstrap-binary
+ "bash"
+ (%current-system))))
+ (d (derivation
+ s
+ "the-thing"
+ b
+ '("--version")
+ #:inputs
+ `((,b))))
+ (o (derivation->output-path d)))
+ (with-derivation-narinfo
+ d
+ (substitutable-path-info s (list o))))))
expected-value: ()
actual-value: (#<<substitutable> path: "/tmp/guix-tests/store/k8yr3vwh9lry8r5x53p1fcpk1d78v4y3-the-thing" deriver: "/tmp/guix-tests/store/1kklp6bxdl34k0qxg4q4wxab7viyl00n-the-thing.drv" refs: () dl-size: 0 nar-size: 1234>)
result: FAIL

test-name: substitutable-paths when substitutes are turned off
location: /tmp/guix-build-guix-1.0.1-5.cc98b00.drv-0/source/tests/store.scm:323
source:
+ (test-equal
+ "substitutable-paths when substitutes are turned off"
+ '()
+ (with-store
+ s
+ (set-build-options s #:use-substitutes? #f)
+ (let* ((b (add-to-store
+ s
+ "bash"
+ #t
+ "sha256"
+ (search-bootstrap-binary
+ "bash"
+ (%current-system))))
+ (d (derivation
+ s
+ "the-thing"
+ b
+ '("--version")
+ #:inputs
+ `((,b))))
+ warning: in 'the-thing': deprecated 'derivation' calling convention used
warning: in 'the-thing': deprecated 'derivation' calling convention used
substitute: warning: authentication and authorization of substitutes disabled!
substitute: guix substitute: warning: ACL for archive imports seems to be uninitialized, substitutes may be unavailable
warning: in 'the-thing': deprecated 'derivation' calling convention used
warning: in 'the-thing': deprecated 'derivation' calling convention used
warning: in 'the-thing': deprecated 'derivation' calling convention used
warning: in 'the-thing': deprecated 'derivation' calling convention used
substitute: warning: authentication and authorization of substitutes disabled!
substitute: guix substitute: warning: ACL for archive imports seems to be uninitialized, substitutes may be unavailable
@ build-started /tmp/guix-tests/store/fh0yssxmiv089bpylfl4v5z4qqq89p56-the-thing.drv - i686-linux /tmp/guix-tests/var/log/guix/drvs/fh//0yssxmiv089bpylfl4v5z4qqq89p56-the-thing.drv.bz2 31208
@ build-succeeded /tmp/guix-tests/store/fh0yssxmiv089bpylfl4v5z4qqq89p56-the-thing.drv -
(o (derivation->output-path d)))
+ (with-derivation-narinfo
+ d
+ (substitutable-paths s (list o))))))
expected-value: ()
actual-value: ("/tmp/guix-tests/store/k8yr3vwh9lry8r5x53p1fcpk1d78v4y3-the-thing")
result: FAIL


When Timothy pushes the fix, I’ll update the ‘guix’ package and check
whether it builds on i686-linux.

Ludo’.
T
T
Timothy Sample wrote on 17 Sep 2019 06:14
Re: bug#37384: Daemon does not honor “useSubsti tutes” on armhf
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 37384-done@debbugs.gnu.org)
877e675zkd.fsf@ngyro.com
Hi,

Ludovic Courtès <ludo@gnu.org> writes:

Toggle quote (2 lines)
> LGTM.

Great! I just pushed it. Thanks for the review.


-- Tim
Closed
?
Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 37384
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch