Opened 10 years ago

Closed 8 years ago

#7766 closed enhancement (fixed)

Upgrade optional spkg valgrind to valgrind-3.7.0

Reported by: jsp Owned by: tbd
Priority: major Milestone: sage-5.0
Component: packages: optional Keywords: memory testing
Cc: timdumol, rlm, iandrus, jpflori, kini Merged in:
Authors: Jaap Spies, Ivan Andrus Reviewers: Jean-Pierre Flori
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

New issue: Valgrind versions prior to 3.7.0 do not build on any Linux 3.x kernels, so we really need to upgrade it ASAP.


The optional valgrind-3.1.1 did not build in Fedora 12 x86_64

reason: glibc-11 is not supported. Even in the last released stable valgrind-3.5.0

So I downloaded from SVN and made an spkg.

See: http://sage.math.washington.edu/home/jsp/valgrind-3.6.0.svn.spkg

Urgent: we need a maintainer other than me and Michael!

Jaap


New spkg (which works on OS X) at http://perso.telecom-paristech.fr/~flori/sage/valgrind-3.7.0.spkg

-Ivan

Attachments (1)

trac_7666_spkg.patch (72.8 KB) - added by jpflori 8 years ago.
spkg diff, for review only

Download all attachments as: .zip

Change History (34)

comment:1 Changed 10 years ago by jsp

  • Status changed from new to needs_review

comment:2 follow-up: Changed 10 years ago by mvngu

  • Cc timdumol added

Ticket #7440 has upgraded the optional Valgrind spkg to version 3.5.0. Tim Dumol has agreed to maintain that spkg.

comment:3 in reply to: ↑ 2 Changed 10 years ago by jsp

Replying to mvngu:

Ticket #7440 has upgraded the optional Valgrind spkg to version 3.5.0. Tim Dumol has agreed to maintain that spkg.

valgrind-3.5.0 didn't compile with Fedora 12 64 bit.

Jaap

comment:4 follow-up: Changed 10 years ago by timdumol

  • Status changed from needs_review to needs_work

It may be worth rebasing this on #7440. spkg-install was updated on #7440 to detect Darwin 9.0 as well, on which Valgrind 3.5.0 works on. Also, mabshoff's name should be removed from the package, as per #7738. Feel free to add your own name to the maintainers list as well. Those issues aside, the package seems to work perfectly here. Nice work.

comment:5 in reply to: ↑ 4 Changed 10 years ago by jsp

Replying to timdumol:

It may be worth rebasing this on #7440. spkg-install was updated on #7440 to detect Darwin 9.0 as well, on which Valgrind 3.5.0 works on. Also, mabshoff's name should be removed from the package, as per #7738. Feel free to add your own name to the maintainers list as well. Those issues aside, the package seems to work perfectly here. Nice work.

Hi Tim,

Would you mind taking over? I'm not a valgrind expert as Michael was.

I have some problems with sage -t on Fedora 12 64 bit on my computer with i7 860 processor. So I tried valgrind. It came up with some issues. See: http://trac.sagemath.org/sage_trac/ticket/7773

Jaap

comment:6 Changed 9 years ago by maldun

Is there an option to install the package via SVN or is the spkg-file in the download link really broken?

There are some empty shell script in the package, I repaired it for my self with copying some of the files from valgrind.3,5.0 package, and it worked fine.

comment:7 Changed 9 years ago by iandrus

  • Description modified (diff)

I created a new spkg from 3.6.1. Since 3.6.1 works on OS X 10.6, I added support for that as well. You can find the spkg at http://boxen.math.washington.edu/home/iandrus/valgrind-3.6.1.p0.spkg I based it off of the spkg at #7440.

One other (perhaps controversial) thing that I did is add suppressions so that starting sage under valgrind and immediately exiting shows no leaks. I was also using the suppression file provided by python which should probably be installed as part of the python spkg-install. Some (or many) of these may be actual leaks, but I don't have the expertise to check them all. Because I don't know if all the suppressions are valid I'm hesitant to mark this as needs_review.

comment:8 Changed 9 years ago by iandrus

  • Cc rlm added

comment:9 Changed 9 years ago by iandrus

  • Status changed from needs_work to needs_review

I have added spkg-test to run valgrind's test suite (which fails for me on OS X 10.6.7).

I created 2 suppression files sage.supp which is the same file as before and sage-liberal.supp which is enough to suppress all the errors caused by sage startup. When testing you can set MEMCHECK_FLAGS to change the suppression file you use. In #11035 I plan to patch sage-valgrind to be able to switch between the two easily.

The default for the moment is to use the same suppression file as before, so it should be 'safer'.

comment:10 Changed 9 years ago by iandrus

  • Authors changed from Jaap Spies to Jaap Spies, Ivan Andrus
  • Cc iandrus added

comment:11 Changed 9 years ago by drkirkby

  • Summary changed from Upgrade optional spkg valgrind to valgrind-3.6.0.svn to Upgrade optional spkg valgrind to valgrind-3.6.1

I changed the title to indicate the ticket now upgrades to 3.6.1 rather than 3.6.0.svn. Personally I think upgrading to svn snapshots is generally a bad idea, as those are less tested than official releases. But I know sometimes its not possible to use a stable release.

Dave

comment:12 Changed 8 years ago by jpflori

  • Cc jpflori added

comment:13 Changed 8 years ago by kini

  • Cc kini added

comment:14 Changed 8 years ago by SimonKing

  • Status changed from needs_review to needs_work

The package does not work for me (on openSUSE 12.1 "Asparagus"). It fails with

checking for a supported OS... ok (linux-gnu)
checking for the kernel version... unsupported (3.1.0-1.2-desktop)
configure: error: Valgrind works on kernels 2.4, 2.6
error configuring valgrind 3.6.1

real    0m2.260s
user    0m0.347s
sys     0m0.382s
************************************************************************
Error installing package valgrind-3.6.1
************************************************************************
Please email sage-devel (http://groups.google.com/group/sage-devel)
explaining the problem and including the relevant part of the log file
  /home/simon/SAGE/sage-4.8.alpha3/spkg/logs/valgrind-3.6.1.log

comment:15 Changed 8 years ago by kini

Yes, this was fixed in 4.7.0. ("275278 valgrind does not build on Linux kernel 3.0.* due to silly" in the changelog). So we apparently need a newer spkg now.

comment:16 Changed 8 years ago by kini

  • Description modified (diff)
  • Summary changed from Upgrade optional spkg valgrind to valgrind-3.6.1 to Upgrade optional spkg valgrind to valgrind-3.7.0

Er, sorry, I meant 3.7.0.

comment:17 Changed 8 years ago by iandrus

  • Status changed from needs_work to needs_review

I created a new spkg from 3.7.0 which can be found at http://boxen.math.washington.edu/home/iandrus/valgrind-3.7.0.spkg

I would like to get this reviewed since the current valgrind package is at 3.3.1 and doesn't support OS X or 3.x linux kernels. I ran some doctests under valgrind on my machine and things worked (well there were lots of leaks, but...).

comment:18 follow-up: Changed 8 years ago by jpflori

  • Reviewers set to Jean-Pierre Flori
  • Status changed from needs_review to needs_work

I guess that make should be replaced by $MAKE in both spkg-check and spkg-install.

The SAGE_LOCAL logic should also be added to spkg-check.

Some message should be printed if tests fail.

The updated changelog in SPKG.txt is wrong (3.6.1 instead of 3.7.0).

I guess the hg log could contain this ticket number at the beginning (#7766: ...)

Moreover the spkg should be named .... .p0.spkg.

Otherwise it looks fine.

I'll test the package now.

comment:19 Changed 8 years ago by jpflori

It runs fine on my quite common amd64 debian system.

comment:20 in reply to: ↑ 18 Changed 8 years ago by iandrus

  • Description modified (diff)
  • Status changed from needs_work to needs_review

Replying to jpflori:

I guess that make should be replaced by $MAKE in both spkg-check and spkg-install.

Done.

The SAGE_LOCAL logic should also be added to spkg-check.

It probably doesn't need it since it will run after spkg-install succeeded, but I added it anyway since it's safer than way. Is there a way to run the tests of an already installed package?

Some message should be printed if tests fail.

Done.

The updated changelog in SPKG.txt is wrong (3.6.1 instead of 3.7.0).

Oops.

I guess the hg log could contain this ticket number at the beginning (#7766: ...)

Good point.

Moreover the spkg should be named .... .p0.spkg.

I thought I read that if there were no patches to upstream then we weren't supposed to put the .p0 on it, but I changed it since I don't really know.

Thanks for taking the time to review this.

comment:21 Changed 8 years ago by iandrus

comment:22 follow-up: Changed 8 years ago by jpflori

About the p0, the problem was that the directory inside the archive was named with a p0 but not the spkg itself so sage would not find it after decompression.

So removing the p0 from both might be a better choice :)

For the check I'm not aware of any way to do that without rebuilding the package but I might have missed something. But if the build dir is deleted after installation that might be impossible anyway.

comment:23 in reply to: ↑ 22 Changed 8 years ago by iandrus

Replying to jpflori:

About the p0, the problem was that the directory inside the archive was named with a p0 but not the spkg itself so sage would not find it after decompression.

So removing the p0 from both might be a better choice :)

Oh I see now. Let me know if you want me to change it. I don't know how big of a deal having a .p0 or not is.

comment:24 Changed 8 years ago by jpflori

Could you just post the diff with the previous patch here, "just for review"?

After that, I'll be completely happy with the ticket.

comment:25 Changed 8 years ago by jpflori

Let's also remove the p0, while we are at it.

comment:26 follow-up: Changed 8 years ago by jpflori

  • Description modified (diff)

I've made the mentioned changes and some minor other changes. I've also updated the descr/license in the SPKG file.

The updated spkg is at http://perso.telecom-paristech/~flori/sage/valgrind-3.7.0.spkg

Ivan: If you don't mind, it may be a good idea to add yourself as a maintainer?

If you do agree, or if you don"t as wel, I'll put this as positive review.

comment:27 in reply to: ↑ 26 Changed 8 years ago by iandrus

  • Description modified (diff)

Replying to jpflori:

I've made the mentioned changes and some minor other changes. I've also updated the descr/license in the SPKG file.

The updated spkg is at http://perso.telecom-paristech/~flori/sage/valgrind-3.7.0.spkg

FWIW I had to add .fr to the server to get the spkg.

Ivan: If you don't mind, it may be a good idea to add yourself as a maintainer?

Yeah, I can be a maintainer. I took your spkg and added myself as maintainer in SPKG.txt. New version is at

http://boxen.math.washington.edu/home/iandrus/valgrind-3.7.0.spkg

Your patches look good BTW.

If you do agree, or if you don"t as wel, I'll put this as positive review.

Excellent. Thanks again.

comment:28 Changed 8 years ago by jpflori

  • Status changed from needs_review to positive_review

comment:29 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:30 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

In SPKG.txt, valgrind_3.7.0 should be valgrind-3.7.0

comment:31 Changed 8 years ago by jpflori

  • Status changed from needs_work to needs_review

I corrected the last version name as well as all the old ones.

Package available at http://perso.telecom-paristech.fr/~flori/sage/valgrind-3.7.0.spkg

Changed 8 years ago by jpflori

spkg diff, for review only

comment:32 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to positive_review

Update the ticket description if you post a new package

comment:33 Changed 8 years ago by jdemeyer

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.