Closed Bug 631841 Opened 10 years ago Closed 10 years ago

Move Valgrind suppression files into mozilla-central

Categories

(Release Engineering :: General, defect)

x86
Linux
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: philor, Assigned: philor)

References

()

Details

Attachments

(2 files, 1 obsolete file)

To get to the happy end-state where people either run Valgrind themselves, or run their patch through the tryserver, and add a suppression for new leaks they expose in broken system libraries, we need something easier to do and to test than "clone the releng tools repo, don't test that you actually copy-pasted correctly and it works because you don't have any way to tryserver it, and do whatever it takes to get a change to the tools repo pushed live, um, I don't actually know what that involves."

It's a little weird to have suppression files that are specific to the OS our build slaves use checked in to mozilla-central, but I think it's worth it to make it reasonable to land something that Valgrind isn't going to like seeing.
(In reply to comment #0)
> To get to the happy end-state where people either run Valgrind themselves, or
> run their patch through the tryserver, and add a suppression for new leaks they
> expose in broken system libraries, we need something easier to do and to test
> than "clone the releng tools repo, don't test that you actually copy-pasted
> correctly and it works because you don't have any way to tryserver it, and do
> whatever it takes to get a change to the tools repo pushed live, um, I don't
> actually know what that involves."
> 
> It's a little weird to have suppression files that are specific to the OS our
> build slaves use checked in to mozilla-central, but I think it's worth it to
> make it reasonable to land something that Valgrind isn't going to like seeing.

Yeah, this is precisely why I didn't put them in there. Also, only a few folks in RelEng can push changes to m-c...although I guess that gets me off the hook for changes!

If we do this, I'd like to think about other possibilities for build-farm information in the tree, e.g. mozconfigs (bug 558180)
Attached patch move 'em (obsolete) — Splinter Review
Assignee: nobody → philringnalda
Status: NEW → ASSIGNED
Attachment #510087 - Flags: review?(ted.mielczarek)
With a little luck, this will actually work.
Attachment #510088 - Flags: review?(catlee)
If we need to pick one of two problems to fix, either "there's no way you can push to either try or mozilla-central with a patch to build/tools, plus there are hundreds of people with mozilla-central access and no idea how to drive a patch into build/tools" or "there are a couple of releng people who might add things to the Valgrind suppression files who don't have mozilla-central access," I'll choose the latter as a thing to fix. We've finally rewritten the access requirements to where we _can_ easily do what we _should_ do, so that problem is a non-problem: 1) find http://www.mozilla.org/hacking/committer/ 2) cc :philor on the bug.
Blocks: 631950
Comment on attachment 510087 [details] [diff] [review]
move 'em

Can you do this without an extra makefile? You can leave the files in build/valgrind, but just install them from build/Makefile. Seems unnecessary. But r=me on whatever you want to do.
Attachment #510087 - Flags: review?(ted.mielczarek) → review+
Comment on attachment 510088 [details] [diff] [review]
use 'em (probably)

Looks ok to me.
Attachment #510088 - Flags: review?(catlee) → review+
That extra makefile would have made more sense if I hadn't been writing a second patch at the same time, where I was going to greater lengths to *avoid* a makefile that just copied files around. Or if I was drunk.

http://hg.mozilla.org/mozilla-central/rev/935706ff9e55
Attachment #510087 - Attachment is obsolete: true
Attachment #511622 - Flags: checked-in+
Comment on attachment 510088 [details] [diff] [review]
use 'em (probably)

Unfortunately, I'm in that class of people I was speaking of, who have absolutely no idea how to go about landing a patch to the tools repo.
Attachment #510088 - Flags: checked-in?
Attachment #511622 - Attachment is patch: true
Attachment #511622 - Attachment mime type: application/octet-stream → text/plain
I screwed that up somehow, not sure how.

/builds/moz2_slave/valgrind-linux/objdir/config/nsinstall -R ../../src/build/valgrind/i686-redhat-linux-gnu.sup ../../src/build/valgrind/x86_64-redhat-linux-gnu.sup ../_valgrind

seems like it would be putting them in objdir/_valgrind/

but 
args: ['/usr/bin/valgrind', '--error-exitcode=1', '--smc-check=all', '--gen-suppressions=all', '--leak-check=full', '--suppressions=_valgrind/i686-redhat-linux-gnu.sup', '/builds/moz2_slave/valgrind-linux/objdir/dist/firefox/firefox-bin', '-no-remote', '-profile', '/builds/moz2_slave/valgrind-linux/objdir/_profile/pgo/pgoprofile/', 'http://localhost:8888/index.html']
...
==11173== Command: /builds/moz2_slave/valgrind-linux/objdir/dist/firefox/firefox-bin -no-remote -profile /builds/moz2_slave/valgrind-linux/objdir/_profile/pgo/pgoprofile/ http://localhost:8888/index.html
==11173==
==11173== FATAL: can't open suppressions file "_valgrind/i686-redhat-linux-gnu.sup"

says either I didn't stick them where I thought I did, or the current directory isn't the objdir like I thought it was, or Valgrind doesn't think my path should be relative to anything, dunno which.
Your revised patch put unnecessary tabs here:
http://hg.mozilla.org/mozilla-central/rev/935706ff9e55#l1.15

two-space indent after continuations please. (You can fix that whenever.)

Also, you're probably getting screwed by this chdir:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py#58

I think you need some abspath-love. Maybe just sticking $PWD before the path in valgrind.sh ?
(In reply to comment #11)
> Your revised patch put unnecessary tabs here:
> http://hg.mozilla.org/mozilla-central/rev/935706ff9e55#l1.15
> 
> two-space indent after continuations please. (You can fix that whenever.)
> 
> Also, you're probably getting screwed by this chdir:
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py#58
> 
> I think you need some abspath-love. Maybe just sticking $PWD before the path in
> valgrind.sh ?

I pushed a change that's already running that adds $PWD to the suppression file.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 672046
Product: mozilla.org → Release Engineering
You need to log in before you can comment on or make changes to this bug.