Opened 12 months ago
Closed 11 months ago
#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 )
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:
Change History (31)
comment:1 Changed 12 months ago by
- Branch set to public/29062-valgrind-python-to-SAGE-LOCAL
comment:2 Changed 12 months ago by
- Commit set to ceb35bb51b8cdc26b99c28fa14079ed718a75a54
- Description modified (diff)
- Status changed from new to needs_review
comment:3 follow-up: ↓ 6 Changed 12 months ago by
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: ↓ 5 Changed 12 months ago by
my Fedora box has /usr/lib64/valgrind/
comment:5 in reply to: ↑ 4 ; follow-up: ↓ 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 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
- Commit changed from ceb35bb51b8cdc26b99c28fa14079ed718a75a54 to 5071b5ddcbe0adee837c70fffffdd450cbbc3312
Branch pushed to git repo; I updated commit sha1. New commits:
5071b5d | src/bin/sage-valgrind: 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 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
- Description modified (diff)
comment:14 Changed 12 months ago by
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.
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: --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
This all looks fine... Are you using sage -valgrind
or are you trying to execute the sage-valgrind
script directly?
comment:19 follow-up: ↓ 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/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
|
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/spkg-install
- should not be relevant for the py2
comment:24 follow-up: ↓ 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 double-check 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/29062-valgrind-python-to-SAGE-LOCAL to e01a340d841414a06b678de09d390b1f600739c9
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
build/pkgs/python3/spkg-install: Install valgrind-python.supp directly in SAGE_LOCAL, not first in SAGE_SRC
src/bin/sage-valgrind: Find python.supp/python3.supp in SAGE_LOCAL/lib/valgrind or system directories