build/pkgs/python3/spkg-install: Install valgrind-python.supp in $SAGE_LOCAL, not in $SAGE_SRC #29062

mkoeppe · 2020-01-21T18:31:27Z

valgrind-python.supp is supplied by the Python source code tarball. Currently our spkg-install copies it to $SAGE_SRC/ext, from where it is copied to $SAGE_LOCAL/share/sage/ext/.

In preparation for #27824 - spkg-configure.m4 for python3:

we change spkg-install to install the file in $SAGE_LOCAL/lib/valgrind/ instead.
make sage -valgrind more flexible in where it expects the file.

This change also has the side effect of removing a peculiar use of SAGE_EXTCODE during building. This simplifies #21785.

Related old tickets:

Be more verbose when running valgrind #19398 (Be more verbose when running valgrind)
Installing local/share/sage/ext/valgrind/python.supp fails #17139 (Installing local/share/sage/ext/valgrind/python.supp fails)
Sage should ship its Valgrind suppressions file, or not insist on its presence #11918 (Sage should ship its Valgrind suppressions file, or not insist on its presence)

CC: @dimpase @embray @rwst @jhpalmieri @jdemeyer @vbraun

Component: build

Author: Matthias Koeppe

Branch/Commit: e01a340

Reviewer: Dima Pasechnik

Issue created by migration from https://trac.sagemath.org/ticket/29062

The text was updated successfully, but these errors were encountered:

mkoeppe · 2020-01-21T19:29:48Z

Branch: public/29062-valgrind-python-to-SAGE-LOCAL

mkoeppe · 2020-01-21T19:32:45Z

Commit: ceb35bb

mkoeppe · 2020-01-21T19:32:45Z

New commits:

`568afdc`	`build/pkgs/python3/spkg-install: Install valgrind-python.supp directly in SAGE_LOCAL, not first in SAGE_SRC`
`ceb35bb`	`src/bin/sage-valgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories`

mkoeppe · 2020-01-21T19:32:45Z

Author: Matthias Koeppe

embray · 2020-01-22T10:56:55Z

comment:3

Makes sense to me, although I don't know about adding the Debian-specific paths. How standard is <prefix>/lib/valgrind/ on other distros? Perhaps the script could just take an optional path to the suppressions file as command-line argument, or have the ability to pass additional command-line arguments directly to valgrind?

dimpase · 2020-01-22T11:10:31Z

comment:4

my Fedora box has /usr/lib64/valgrind/

mkoeppe · 2020-01-22T13:49:04Z

comment:5

Replying to @dimpase:

my Fedora box has /usr/lib64/valgrind/

OK I'll add that.

mkoeppe · 2020-01-22T13:50:53Z

comment:6

Replying to @embray:

Makes sense to me, although I don't know about adding the Debian-specific paths. How standard is <prefix>/lib/valgrind/ on other distros?

As far as I can see, <prefix>/lib/valgrind/ is the install location of upstream valgrind. It has the default suppression file. It's a natural place for distros to install additional things into.

Perhaps the script could just take an optional path to the suppressions file as command-line argument, or have the ability to pass additional command-line arguments directly to valgrind?

A follow-up ticket.

sagetrac-git · 2020-01-22T16:18:07Z

Changed commit from ceb35bb to 5071b5d

sagetrac-git · 2020-01-22T16:18:07Z

Branch pushed to git repo; I updated commit sha1. New commits:

`5071b5d`	`src/bin/sage-valgrind: Also check /usr/lib64/valgrind`

mkoeppe · 2020-01-24T16:00:33Z

comment:8

Ready for review

mkoeppe · 2020-01-28T18:13:51Z

comment:10

Replying to @mkoeppe:

Replying to @dimpase:

my Fedora box has /usr/lib64/valgrind/

OK I'll add that.

This is added and the ticket is waiting for review...

dimpase · 2020-01-29T13:56:49Z

comment:11

How can this be tested?

mkoeppe · 2020-01-29T14:11:12Z

comment:12

Check that sage-valgrind works without error in a fresh build from a distclean source, both in a python2 and python3 build.

dimpase · 2020-01-31T18:37:32Z

comment:14

this is what I see on py3 on Debian 10

(sage-sh) dimpase@penguin:sage-src$ sage-valgrind 
Using default flags: --leak-resolution=high --leak-check=full --num-callers=25  --suppressions=/usr/lib/valgrind/python3.supp --suppressions=/home/dimpase/sage-src/local/share/sage/ext/valgrind/pyalloc.supp --suppressions=/home/dimpase/sage-src/local/share/sage/ext/valgrind/sage.supp --suppressions=/home/dimpase/sage-src/local/share/sage/ext/valgrind/sage-additional.supp
==17460== Memcheck, a memory error detector
==17460== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==17460== Using Valgrind-3.14.0 and LibVEX; rerun with -h for copyright info
==17460== Command: /home/dimpase/sage-src/src/bin/sage-ipython -i
==17460== 
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.1.beta2, Release Date: 2020-01-26               │
│ Using Python 3.7.3. Type "help()" for help.                        │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: 2+2
4
sage:                                                                                                                                                                                     
Exiting Sage (CPU time 0m0.17s, Wall time 2m5.33s).
(sage-sh) dimpase@penguin:sage-src$

Is this what's expected?

PS. sage --valgrind produces the same output.

mkoeppe · 2020-01-31T20:05:23Z

comment:15

Yes, looks right.

dimpase · 2020-01-31T21:15:50Z

comment:16

I'll run a whole build of this branch on py2, just to be sure.

dimpase · 2020-01-31T23:39:24Z

comment:17

On py2 it does not seem to get installed in the right place:

$ ./sage --valgrind
Python suppressions not found (not installed?), skipping
Using default flags: --leak-resolution=high --leak-check=full --num-callers=25  --suppressions=/home/scratch2/dimpase/sage/sage-clang/local/share/sage/ext/valgrind/pyalloc.supp --suppressions=/home/scratch2/dimpase/sage/sage-clang/local/share/sage/ext/valgrind/sage.supp --suppressions=/home/scratch2/dimpase/sage/sage-clang/local/share/sage/ext/valgrind/sage-additional.supp
==21807== Memcheck, a memory error detector
==21807== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==21807== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==21807== Command: /home/scratch2/dimpase/sage/sage-clang/src/bin/sage-ipython -i
==21807== 
┌────────────────────────────────────────────────────────────────────┐
│ SageMath version 9.1.beta2, Release Date: 2020-01-26               │
│ Using Python 2.7.15. Type "help()" for help.                       │
└────────────────────────────────────────────────────────────────────┘
┏━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Warning: this is a prerelease version, and it may be unstable.     ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
sage: 2+2
4
sage: exit
Exiting Sage (CPU time 0m0.11s, Wall time 0m5.92s).

In install log I see

cp <SAGEROOT>/src/ext/valgrind/sage.supp <SAGEROOT>/local/share/sage/ext/valgrind/sage.supp

and the same with files pyalloc.supp, sage-additional.supp.

And then

[python2-2.7.15.p1] Installing valgrind suppression file...
[python2-2.7.15.p1] Misc/valgrind-python.supp -> <SAGELOCAL>/var/tmp/sage/build/python2-2.7.15.p1/inst/home/scratch2/dimpase/sage/sage-clang/local/var/tmp/sage/build/python2-2.7.15.p1/inst/home/scratch2/dimpase/sage/sage-clang/local/lib/valgrind/python.supp

Is this branch missing needed py2 changes?

mkoeppe · 2020-01-31T23:53:42Z

comment:18

This all looks fine... Are you using sage -valgrind or are you trying to execute the sage-valgrind script directly?

mkoeppe · 2020-01-31T23:54:52Z

comment:19

Does the local/lib/valgrind/python.supp file exist after installation?

dimpase · 2020-02-01T00:03:07Z

comment:20

Replying to @mkoeppe:

Does the local/lib/valgrind/python.supp file exist after installation?

no.

sagetrac-git · 2020-02-01T00:10:23Z

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

`5e86aad`	`build/pkgs/python3/spkg-install: Install valgrind-python.supp directly in SAGE_LOCAL, not first in SAGE_SRC`
`52552f3`	`src/bin/sage-valgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories`
`ae6b748`	`src/bin/sage-valgrind: Also check /usr/lib64/valgrind`
`e01a340`	`Fix up valgrind-python.supp install dir`

sagetrac-git · 2020-02-01T00:10:23Z

Changed commit from 5071b5d to e01a340

mkoeppe · 2020-02-01T00:11:49Z

comment:22

Thanks for catching this.

dimpase · 2020-02-01T00:28:53Z

comment:23

have you fixed the problem? I only see a change in
build/pkgs/python3/spkg-install - should not be relevant for the py2

mkoeppe · 2020-02-01T00:30:18Z

comment:24

Py2 has a symlink to the same script

dimpase · 2020-02-01T00:41:14Z

comment:25

Replying to @mkoeppe:

Py2 has a symlink to the same script

this is evil :-)

mkoeppe · 2020-02-01T00:57:40Z

comment:26

Wasn't me :)

dimpase · 2020-02-01T01:28:45Z

comment:27

OK, good, this works for py2.

I'll double-check for py3 - the previous check might have been faulty due to incomplete cleaning...

mkoeppe · 2020-02-01T01:29:29Z

comment:28

Thanks!

dimpase · 2020-02-01T08:36:10Z

Reviewer: Dima Pasechnik

dimpase · 2020-02-01T08:36:10Z

comment:29

looks good.

mkoeppe · 2020-02-01T13:45:40Z

comment:30

Thanks for reviewing!

vbraun · 2020-02-10T20:05:50Z

Changed branch from public/29062-valgrind-python-to-SAGE-LOCAL to e01a340

mkoeppe added this to the sage-9.1 milestone Jan 21, 2020

mkoeppe added c: build labels Jan 21, 2020

This comment has been minimized.

Sign in to view

mkoeppe added the s: needs review label Jan 21, 2020

This comment has been minimized.

Sign in to view

dimpase added s: positive review and removed s: needs review labels Feb 1, 2020

vbraun removed the s: positive review label Feb 10, 2020

vbraun closed this as completed in 3c75a3a Feb 10, 2020

build/pkgs/python3/spkg-install: Install valgrind-python.supp in $SAGE_LOCAL, not in $SAGE_SRC #29062

build/pkgs/python3/spkg-install: Install valgrind-python.supp in $SAGE_LOCAL, not in $SAGE_SRC #29062

Comments

mkoeppe commented Jan 21, 2020

mkoeppe commented Jan 21, 2020

This comment has been minimized.

mkoeppe commented Jan 21, 2020

mkoeppe commented Jan 21, 2020

mkoeppe commented Jan 21, 2020

embray commented Jan 22, 2020

dimpase commented Jan 22, 2020

mkoeppe commented Jan 22, 2020

mkoeppe commented Jan 22, 2020

sagetrac-git mannequin commented Jan 22, 2020

sagetrac-git mannequin commented Jan 22, 2020

mkoeppe commented Jan 24, 2020

mkoeppe commented Jan 28, 2020

dimpase commented Jan 29, 2020

mkoeppe commented Jan 29, 2020

This comment has been minimized.

dimpase commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

dimpase commented Jan 31, 2020

dimpase commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

mkoeppe commented Jan 31, 2020

dimpase commented Feb 1, 2020

sagetrac-git mannequin commented Feb 1, 2020

sagetrac-git mannequin commented Feb 1, 2020

mkoeppe commented Feb 1, 2020

dimpase commented Feb 1, 2020

mkoeppe commented Feb 1, 2020

dimpase commented Feb 1, 2020

mkoeppe commented Feb 1, 2020

dimpase commented Feb 1, 2020

mkoeppe commented Feb 1, 2020

dimpase commented Feb 1, 2020

dimpase commented Feb 1, 2020

mkoeppe commented Feb 1, 2020

vbraun commented Feb 10, 2020