Valgrind blocks R on powerpc64le

  • Done
  • quality assurance status badge
Details
2 participants
  • Andreas Enge
  • Simon Tournier
Owner
unassigned
Submitted by
Andreas Enge
Severity
normal
A
A
Andreas Enge wrote on 19 Apr 2023 22:08
(address . bug-guix@gnu.org)
ZEBKQP+y4Q0dUV0r@jurong
Currently r-minimal depends on texlive-latex-xkeyval, which depends on
texlive-ms, which for a reason I do not see pulls in the valgrind variable.

This is at version 3.17, which fails on powerpc64le. The user facing
variable valgrind/interactive, however, is at 3.20, and it builds.

After the impending core-updates merge, we should update valgrind to
3.20.

I am opening a bug so that this is not forgotten.

Andreas
S
S
Simon Tournier wrote on 20 Apr 2023 13:26
87r0sedcb6.fsf@gmail.com
Hi,

On mer., 19 avril 2023 at 22:08, Andreas Enge <andreas@enge.fr> wrote:

Toggle quote (3 lines)
> Currently r-minimal depends on texlive-latex-xkeyval, which depends on
> texlive-ms, which for a reason I do not see pulls in the valgrind variable.

Toggle snippet (8 lines)
$ ./pre-inst-env guix graph --path r-minimal texlive-ms
r-minimal@4.2.3
texlive-updmap.cfg@59745
texlive-latex-xkeyval@59745
texlive-pgf@59745
texlive-ms@59745

But I do not see either how valgrind enters in the picture as a
dependency for r-minimal,

Toggle snippet (4 lines)
$ ./pre-inst-env guix graph --path r-minimal -e '(@@ (gnu packages valgrind) valgrind)' -t bag -s powerpc64le-linux
guix graph: error: no path from 'r-minimal@4.2.3' to 'valgrind@3.17.0'

However, please note:

Toggle snippet (29 lines)
$ for p in $(./pre-inst-env guix refresh -l -e '(@@ (gnu packages valgrind) valgrind)' | cut -f2 -d':'); do echo $p ;done | grep '^r\-'
r-bigmelon@1.24.0
r-multidplyr@0.1.3
r-cistopic-next@0.3.0-1.04cecbb
r-cistopic@2.1.0
r-chromunity@0.0.1-1.09fce8b
r-cicero-monocle3@1.3.2-1.fa2fb65
r-tmaptools@3.1-1
r-zonebuilder@0.0.2
r-chipseeker@1.34.1
r-snapatac@2.0
r-rastervis@0.51.5
r-insol@1.2.2
r-bien@1.2.6
r-sungeo@0.2.292
r-leaflet@2.1.2
r-spectre@0.5.5-1.f6648ab
r-zonator@0.6.0
r-zoon@0.6.5
r-biocdockermanager@1.10.0
r-irkernel@1.3.2
r-prereg@0.6.0
r-doubletcollection@1.1.0-1.c0d62f1
r-rmpi@0.7-1
r-pbdmpi@0.4-6
r-torch@0.10.0
r-ctrdata@1.12.1

and I guess that’s what you are observing. Somehow, something like,

Toggle snippet (3 lines)
for q in $(for p in $(./pre-inst-env guix refresh -l -e '(@@ (gnu packages valgrind) valgrind)' | cut -f2 -d':' | sort); do echo $p ;done | grep '^r\-'); do echo "# $q";./pre-inst-env guix graph --path $q -e '(@@ (gnu packages valgrind) valgrind)' ;done | grep -B 2 valgrind

shows that most of the paths do not involve texlive-ms. Instead, it
seems related to lz4 or openmpi or jq.

Toggle quote (3 lines)
> This is at version 3.17, which fails on powerpc64le. The user facing
> variable valgrind/interactive, however, is at 3.20, and it builds.

As far I can see, it is hard to cross-compile since substitutes are
missing. Well, maybe the CI is not building them. I do not know.


Toggle quote (3 lines)
> After the impending core-updates merge, we should update valgrind to
> 3.20.

Note the update of valgrind is not “so much“. :-)

Toggle snippet (4 lines)
$ ./pre-inst-env guix refresh -l -e '(@@ (gnu packages valgrind) valgrind)' | cut -f1 -d':'
Building the following 569 packages would ensure 1169 dependent packages are rebuilt

Well, some packages are intensive to rebuild as ungoogled-chromium but I
guess that if these:

23 jq@1.6
37 qtwebengine@5.15.8
39 openmpi@4.1.4
45 dtc@1.6.1
405 lz4@1.9.3

passes with valgrind at 3.20, we should almost be good, IMHO. :-)

Most of the 569 packages are rebuilt because lz4 is rebuild. Well, I am
giving a try… If it is not part of the next core-updates merge, then
using a feature branch building the substitutes, the update of valgrind
could go to master. ;-)


Cheers,
simon
A
A
Andreas Enge wrote on 20 Apr 2023 13:50
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 62954@debbugs.gnu.org)
ZEEm+k0zvdjripae@jurong
Am Thu, Apr 20, 2023 at 01:26:37PM +0200 schrieb Simon Tournier:
Toggle quote (3 lines)
> shows that most of the paths do not involve texlive-ms. Instead, it
> seems related to lz4 or openmpi or jq.

So I start to understand. lz4 depends on valgrind. I do not know why.
It is given as a native input, so probably the tests require valgrind?
If yes, that looks a bit excessive to me - the developers of lz4 are very
welcome to check for memory leaks from time to time, but doing this at
every compilation is excessive. What do you think of dropping the valgrind
input (at the same time as updating valgrind, say)? It does not seem to be
necessary, as it is already dropped on architectures that do not provide it,
without further ado. Maybe this is what hides it from "guix refresh" and
"guix graph"?

Then texlive-ms depends on lz4 and indirectly on valgrind:
$ ./pre-inst-env guix build --system=powerpc64le-linux texlive-ms -n
/gnu/store/j0wzdc36vvgvj4zh49a71sc115m6m76b-texlive-ms-59745.drv
/gnu/store/jl6p6m1zngi1rjl2808zvnb9wpiphhjf-texlive-ms-59745-checkout.drv
/gnu/store/gfkdb5pys2b8dr2mqgs5gbwfflwlc4kh-subversion-1.14.2.drv
/gnu/store/xw5kdli3i92iwd7wpplcb0p89g1p3a29-lz4-1.9.3.drv
/gnu/store/z6kf2pg48b9a87angabkfyfv4knhhwjy-valgrind-3.17.0.drv

More precisely: texlive-ms is downloaded via subversion via
simple-texlive-package in (gnu packages texlive), which relies on
texlive-origin from (guix build-system texlive).

$ ./pre-inst-env guix build --system=powerpc64le-linux subversion -n
The following derivations would be built:
/gnu/store/gfkdb5pys2b8dr2mqgs5gbwfflwlc4kh-subversion-1.14.2.drv
/gnu/store/xw5kdli3i92iwd7wpplcb0p89g1p3a29-lz4-1.9.3.drv
/gnu/store/z6kf2pg48b9a87angabkfyfv4knhhwjy-valgrind-3.17.0.drv

So subversion depends on valgrind! And all new simplex-texlive-packages
are concerned.

I think the solution is to indeed remove valgrind from the native inputs
of lz4.

And I think there is a shortcoming in "guix refresh" that it does not
take the source code of packages into account.

What do you think?

Andreas
S
S
Simon Tournier wrote on 20 Apr 2023 14:35
(name . Andreas Enge)(address . andreas@enge.fr)(address . 62954@debbugs.gnu.org)
87o7nid941.fsf@gmail.com
Hi,

On jeu., 20 avril 2023 at 13:50, Andreas Enge <andreas@enge.fr> wrote:

Toggle quote (3 lines)
> So subversion depends on valgrind! And all new simplex-texlive-packages
> are concerned.

Good catch! :-)

Toggle quote (3 lines)
> I think the solution is to indeed remove valgrind from the native inputs
> of lz4.

Yeah, and I think it’s safe. :-) Considering,

Toggle snippet (4 lines)
starting phase `check'
CFLAGS="" LDFLAGS="" make -C tests test

and the ’test’ suite reads,

Toggle snippet (4 lines)
$ grep '^test:' $(./pre-inst-env guix build lz4 -S)/tests/Makefile
test: test-lz4 test-lz4c test-frametest test-fullbench test-fuzzer test-install test-amalgamation listTest test-decompress-partial

Last, ’valgrind’ appears only in this rule:

Toggle snippet (21 lines)
test-mem: lz4 datagen fuzzer frametest fullbench
@echo "\n ---- valgrind tests : memory analyzer ----"
valgrind --leak-check=yes --error-exitcode=1 $(DATAGEN) -g50M > $(VOID)
$(DATAGEN) -g16KB > ftmdg16K
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) -9 -BD -f ftmdg16K $(VOID)
$(DATAGEN) -g16KB -s2 > ftmdg16K2
$(DATAGEN) -g16KB -s3 > ftmdg16K3
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) --force --multiple ftmdg16K ftmdg16K2 ftmdg16K3
$(DATAGEN) -g7MB > ftmdg7M
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) -9 -B5D -f ftmdg7M ftmdg16K2
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) -t ftmdg16K2
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) -bi1 ftmdg7M
valgrind --leak-check=yes --error-exitcode=1 ./fullbench -i1 ftmdg7M ftmdg16K2
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) -B4D -f -vq ftmdg7M $(VOID)
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) --list -m ftm*.lz4
valgrind --leak-check=yes --error-exitcode=1 $(LZ4) --list -m -v ftm*.lz4
$(RM) ftm*
valgrind --leak-check=yes --error-exitcode=1 ./fuzzer -i64 -t1
valgrind --leak-check=yes --error-exitcode=1 ./frametest -i256

so ’valgrind’ is never used by the current recipe. Ahah! :-D

And it’s confirmed: lz4 builds fine on core-updates for x68_64.

Therefore, I think this change before the merge is worth:

Toggle snippet (20 lines)
1 file changed, 1 insertion(+), 5 deletions(-)
gnu/packages/compression.scm | 6 +-----

modified gnu/packages/compression.scm
@@ -840,11 +840,7 @@ (define-public lz4
(build-system gnu-build-system)
(outputs (list "out" "static"))
(native-inputs
- (append
- (list python) ;; For tests.
- (if (member (%current-system) (package-supported-systems valgrind))
- (list valgrind)
- '())))
+ (list python))
(arguments
`(;; Not designed for parallel testing.
;; See https://github.com/lz4/lz4/issues/957#issuecomment-737419821


Cheers,
simon
S
S
Simon Tournier wrote on 20 Apr 2023 15:25
(name . Andreas Enge)(address . andreas@enge.fr)(address . 62954@debbugs.gnu.org)
CAJ3okZ2kwzc057v+8Y=qD8O9nw-PX=Q1HJ9mgP6L83H=u5SQDw@mail.gmail.com
Hi Andreas,

On Thu, 20 Apr 2023 at 13:50, Andreas Enge <andreas@enge.fr> wrote:

Toggle quote (3 lines)
> I think the solution is to indeed remove valgrind from the native inputs
> of lz4.

See #62967 [1] for an attempt. I am rebuilding to detect the potential problem.


Cheers,
simon
A
A
Andreas Enge wrote on 20 Apr 2023 18:55
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 62954@debbugs.gnu.org)
ZEFuldg6prmdzkAX@jurong
Am Thu, Apr 20, 2023 at 03:25:17PM +0200 schrieb Simon Tournier:
Toggle quote (2 lines)
> See #62967 [1] for an attempt. I am rebuilding to detect the potential problem.

Not sure why we need a second issue... All this should work, let us wait
till after the core-updates merge.

Andreas
S
S
Simon Tournier wrote on 21 Apr 2023 09:59
(name . Andreas Enge)(address . andreas@enge.fr)(address . 62954@debbugs.gnu.org)
86fs8t64ye.fsf@gmail.com
Hi,

On Thu, 20 Apr 2023 at 18:55, Andreas Enge <andreas@enge.fr> wrote:
Toggle quote (6 lines)
> Am Thu, Apr 20, 2023 at 03:25:17PM +0200 schrieb Simon Tournier:
>> See #62967 [1] for an attempt. I am rebuilding to detect the potential problem.
>
> Not sure why we need a second issue... All this should work, let us wait
> till after the core-updates merge.

Because it is two “mailing lists“ under the hood: guix-patches and bug-guix.

As someone who digs (time to time, and not enough these days, oups!) to
“forgotten” issues, I find easier to follow the discussion about one
specific bug and the discussion about one specific implementation
when they are separated – somehow it reduces the “noise”.

Well, for example consider #58650 [1]. It is time-consuming to know if
the PATCH is “forgotten“ or if it does not fix the issue and can be
dropped.

Moreover, the two patches of #58650 [1] does not appear if you follow
only guix-patches; because the issue had not been marked +patch.

Speaking about “forgotten” issues, #62967 (patch) does not mention
#62954 (bug) and that is a kind of “mistake”, I agree. :-)

Last, on the pragmatic side, I do not know if the CI is following
bug-guix and if it tries to extract patches from it. (Yes, for this
case it is not relevant; the patch is dropped by CI because it implies
more rebuilds than the threshold.)

Well, it’s a matter of taste. :-)



Cheers,
simon
A
A
Andreas Enge wrote on 21 Apr 2023 10:37
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 62954@debbugs.gnu.org)
ZEJLOCCteEjKWAzU@jurong
Am Fri, Apr 21, 2023 at 09:59:37AM +0200 schrieb Simon Tournier:
Toggle quote (4 lines)
> Because it is two “mailing lists“ under the hood: guix-patches and bug-guix.
> Last, on the pragmatic side, I do not know if the CI is following
> bug-guix and if it tries to extract patches from it.

Ah indeed, I suppose it does not, thanks for the explanation.

Andreas
A
A
Andreas Enge wrote on 25 Apr 2023 17:24
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 62954-done@debbugs.gnu.org)
ZEfwldWFd1vH8Elu@jurong
Closing this bug, the topic is treated through the patches in

Andreas
Closed
?
Your comment

This issue is archived.

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

To respond to this issue using the mumi CLI, first switch to it
mumi current 62954
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