#29062 closed enhancement (fixed)

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

Reported by: mkoeppe Owned by:
Priority: major Milestone: sage-9.1
Component: build Keywords:
Cc: dimpase, embray, rws, jhpalmieri, jdemeyer, vbraun Merged in:
Authors: Matthias Koeppe Reviewers: Dima Pasechnik
Report Upstream: N/A Work issues:
Branch: e01a340 (Commits) Commit: e01a340d841414a06b678de09d390b1f600739c9
Dependencies: Stopgaps:

Description (last modified by mkoeppe)

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:

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

Change History (31)

comment:1 Changed 12 months ago by mkoeppe

  • Branch set to public/29062-valgrind-python-to-SAGE-LOCAL

comment:2 Changed 12 months ago by mkoeppe

  • Authors set to Matthias Koeppe
  • Commit set to ceb35bb51b8cdc26b99c28fa14079ed718a75a54
  • Description modified (diff)
  • Status changed from new to needs_review

New commits:

568afdcbuild/pkgs/python3/spkg-install: Install valgrind-python.supp directly in SAGE_LOCAL, not first in SAGE_SRC
ceb35bbsrc/bin/sage-valgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories

comment:3 follow-up: Changed 12 months ago by 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? 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?

comment:4 follow-up: Changed 12 months ago by dimpase

my Fedora box has /usr/lib64/valgrind/

comment:5 in reply to: ↑ 4 ; follow-up: Changed 12 months ago by mkoeppe

Replying to dimpase:

my Fedora box has /usr/lib64/valgrind/

OK I'll add that.

comment:6 in reply to: ↑ 3 Changed 12 months ago by mkoeppe

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.

comment:7 Changed 12 months ago by git

  • Commit changed from ceb35bb51b8cdc26b99c28fa14079ed718a75a54 to 5071b5ddcbe0adee837c70fffffdd450cbbc3312

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

5071b5dsrc/bin/sage-valgrind: Also check /usr/lib64/valgrind

comment:8 Changed 12 months ago by mkoeppe

Ready for review

comment:9 Changed 12 months ago by mkoeppe

  • Cc vbraun added

comment:10 in reply to: ↑ 5 Changed 12 months ago by mkoeppe

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...

comment:11 Changed 12 months ago by dimpase

How can this be tested?

comment:12 Changed 12 months ago by mkoeppe

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

comment:13 Changed 12 months ago by mkoeppe

  • Description modified (diff)

comment:14 Changed 12 months ago by dimpase

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.

Last edited 12 months ago by dimpase (previous) (diff)

comment:15 Changed 12 months ago by mkoeppe

Yes, looks right.

comment:16 Changed 12 months ago by dimpase

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

comment:17 Changed 12 months ago by dimpase

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?

comment:18 Changed 12 months ago by mkoeppe

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

comment:19 follow-up: Changed 12 months ago by mkoeppe

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

comment:20 in reply to: ↑ 19 Changed 12 months ago by dimpase

Replying to mkoeppe:

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

no.

comment:21 Changed 12 months ago by git

  • Commit changed from 5071b5ddcbe0adee837c70fffffdd450cbbc3312 to e01a340d841414a06b678de09d390b1f600739c9

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

5e86aadbuild/pkgs/python3/spkg-install: Install valgrind-python.supp directly in SAGE_LOCAL, not first in SAGE_SRC
52552f3src/bin/sage-valgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories
ae6b748src/bin/sage-valgrind: Also check /usr/lib64/valgrind
e01a340Fix up valgrind-python.supp install dir

comment:22 Changed 12 months ago by mkoeppe

Thanks for catching this.

comment:23 Changed 12 months ago by dimpase

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

comment:24 follow-up: Changed 12 months ago by mkoeppe

Py2 has a symlink to the same script

comment:25 in reply to: ↑ 24 Changed 12 months ago by dimpase

Replying to mkoeppe:

Py2 has a symlink to the same script

this is evil :-)

comment:26 Changed 12 months ago by mkoeppe

Wasn't me :)

comment:27 Changed 12 months ago by dimpase

OK, good, this works for py2.

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

comment:28 Changed 12 months ago by mkoeppe

Thanks!

comment:29 Changed 12 months ago by dimpase

  • Reviewers set to Dima Pasechnik
  • Status changed from needs_review to positive_review

looks good.

comment:30 Changed 12 months ago by mkoeppe

Thanks for reviewing!

comment:31 Changed 11 months ago by vbraun

  • Branch changed from public/29062-valgrind-python-to-SAGE-LOCAL to e01a340d841414a06b678de09d390b1f600739c9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.