Opened 12 months ago
Closed 11 months ago
#29062 closed enhancement (fixed)
build/pkgs/python3/spkginstall: Install valgrindpython.supp in $SAGE_LOCAL, not in $SAGE_SRC
Reported by:  mkoeppe  Owned by:  

Priority:  major  Milestone:  sage9.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 )
valgrindpython.supp
is supplied by the Python source code tarball. Currently our spkginstall
copies it to $SAGE_SRC/ext
, from where it is copied to $SAGE_LOCAL/share/sage/ext/
.
In preparation for #27824  spkgconfigure.m4 for python3:
 we change
spkginstall
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:
Change History (31)
comment:1 Changed 12 months ago by
 Branch set to public/29062valgrindpythontoSAGELOCAL
comment:2 Changed 12 months ago by
 Commit set to ceb35bb51b8cdc26b99c28fa14079ed718a75a54
 Description modified (diff)
 Status changed from new to needs_review
comment:3 followup: ↓ 6 Changed 12 months ago by
Makes sense to me, although I don't know about adding the Debianspecific paths. How standard is <prefix>/lib/valgrind/
on other distros? Perhaps the script could just take an optional path to the suppressions file as commandline argument, or have the ability to pass additional commandline arguments directly to valgrind?
comment:4 followup: ↓ 5 Changed 12 months ago by
my Fedora box has /usr/lib64/valgrind/
comment:5 in reply to: ↑ 4 ; followup: ↓ 10 Changed 12 months ago by
comment:6 in reply to: ↑ 3 Changed 12 months ago by
Replying to embray:
Makes sense to me, although I don't know about adding the Debianspecific 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 commandline argument, or have the ability to pass additional commandline arguments directly to valgrind?
A followup ticket.
comment:7 Changed 12 months ago by
 Commit changed from ceb35bb51b8cdc26b99c28fa14079ed718a75a54 to 5071b5ddcbe0adee837c70fffffdd450cbbc3312
Branch pushed to git repo; I updated commit sha1. New commits:
5071b5d  src/bin/sagevalgrind: Also check /usr/lib64/valgrind

comment:8 Changed 12 months ago by
Ready for review
comment:9 Changed 12 months ago by
 Cc vbraun added
comment:10 in reply to: ↑ 5 Changed 12 months ago by
comment:11 Changed 12 months ago by
How can this be tested?
comment:12 Changed 12 months ago by
Check that sagevalgrind 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
 Description modified (diff)
comment:14 Changed 12 months ago by
this is what I see on py3 on Debian 10
(sagesh) dimpase@penguin:sagesrc$ sagevalgrind Using default flags: leakresolution=high leakcheck=full numcallers=25 suppressions=/usr/lib/valgrind/python3.supp suppressions=/home/dimpase/sagesrc/local/share/sage/ext/valgrind/pyalloc.supp suppressions=/home/dimpase/sagesrc/local/share/sage/ext/valgrind/sage.supp suppressions=/home/dimpase/sagesrc/local/share/sage/ext/valgrind/sageadditional.supp ==17460== Memcheck, a memory error detector ==17460== Copyright (C) 20022017, and GNU GPL'd, by Julian Seward et al. ==17460== Using Valgrind3.14.0 and LibVEX; rerun with h for copyright info ==17460== Command: /home/dimpase/sagesrc/src/bin/sageipython i ==17460== ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 9.1.beta2, Release Date: 20200126 │ │ 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). (sagesh) dimpase@penguin:sagesrc$
Is this what's expected?
PS. sage valgrind
produces the same output.
comment:15 Changed 12 months ago by
Yes, looks right.
comment:16 Changed 12 months ago by
I'll run a whole build of this branch on py2, just to be sure.
comment:17 Changed 12 months ago by
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: leakresolution=high leakcheck=full numcallers=25 suppressions=/home/scratch2/dimpase/sage/sageclang/local/share/sage/ext/valgrind/pyalloc.supp suppressions=/home/scratch2/dimpase/sage/sageclang/local/share/sage/ext/valgrind/sage.supp suppressions=/home/scratch2/dimpase/sage/sageclang/local/share/sage/ext/valgrind/sageadditional.supp ==21807== Memcheck, a memory error detector ==21807== Copyright (C) 20022017, and GNU GPL'd, by Julian Seward et al. ==21807== Using Valgrind3.13.0 and LibVEX; rerun with h for copyright info ==21807== Command: /home/scratch2/dimpase/sage/sageclang/src/bin/sageipython i ==21807== ┌────────────────────────────────────────────────────────────────────┐ │ SageMath version 9.1.beta2, Release Date: 20200126 │ │ 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
, sageadditional.supp
.
And then
[python22.7.15.p1] Installing valgrind suppression file... [python22.7.15.p1] Misc/valgrindpython.supp > <SAGELOCAL>/var/tmp/sage/build/python22.7.15.p1/inst/home/scratch2/dimpase/sage/sageclang/local/var/tmp/sage/build/python22.7.15.p1/inst/home/scratch2/dimpase/sage/sageclang/local/lib/valgrind/python.supp
Is this branch missing needed py2 changes?
comment:18 Changed 12 months ago by
This all looks fine... Are you using sage valgrind
or are you trying to execute the sagevalgrind
script directly?
comment:19 followup: ↓ 20 Changed 12 months ago by
Does the local/lib/valgrind/python.supp
file exist after installation?
comment:20 in reply to: ↑ 19 Changed 12 months ago by
comment:21 Changed 12 months ago by
 Commit changed from 5071b5ddcbe0adee837c70fffffdd450cbbc3312 to e01a340d841414a06b678de09d390b1f600739c9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
5e86aad  build/pkgs/python3/spkginstall: Install valgrindpython.supp directly in SAGE_LOCAL, not first in SAGE_SRC

52552f3  src/bin/sagevalgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories

ae6b748  src/bin/sagevalgrind: Also check /usr/lib64/valgrind

e01a340  Fix up valgrindpython.supp install dir

comment:22 Changed 12 months ago by
Thanks for catching this.
comment:23 Changed 12 months ago by
have you fixed the problem? I only see a change in
build/pkgs/python3/spkginstall
 should not be relevant for the py2
comment:24 followup: ↓ 25 Changed 12 months ago by
Py2 has a symlink to the same script
comment:25 in reply to: ↑ 24 Changed 12 months ago by
comment:26 Changed 12 months ago by
Wasn't me :)
comment:27 Changed 12 months ago by
OK, good, this works for py2.
I'll doublecheck for py3  the previous check might have been faulty due to incomplete cleaning...
comment:28 Changed 12 months ago by
Thanks!
comment:29 Changed 12 months ago by
 Reviewers set to Dima Pasechnik
 Status changed from needs_review to positive_review
looks good.
comment:30 Changed 12 months ago by
Thanks for reviewing!
comment:31 Changed 11 months ago by
 Branch changed from public/29062valgrindpythontoSAGELOCAL to e01a340d841414a06b678de09d390b1f600739c9
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
build/pkgs/python3/spkginstall: Install valgrindpython.supp directly in SAGE_LOCAL, not first in SAGE_SRC
src/bin/sagevalgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories