Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Valgrind check disable, Trait "UniqueStr" rename, #3774

Closed
wants to merge 2 commits into from
Closed

Valgrind check disable, Trait "UniqueStr" rename, #3774

wants to merge 2 commits into from

Conversation

vertexclique
Copy link
Member

2aa1870 : This disables valgrind tests and argument I think

@vertexclique
Copy link
Member Author

hash 629221f is for issue #3712

@bblum
Copy link
Contributor

bblum commented Oct 19, 2012

I feel quite strongly that disabling valgrind on tests by default is not something we want to do.

after reading #3771, i reconsider. It matters that the bots run the test suite with valgrind, but locally everywhere could be a different story.

@vertexclique
Copy link
Member Author

If you don't want to merge first commit you can do second commit, later we can reconsider first commit I think :)

@catamorphism
Copy link
Contributor

@brson said that the bots would have to be modified before we disable valgrind in the tests (presumably to run valgrind if we're running on the bots), so I'm not merging the first commit yet.

Also, in future commits, can you set the target branch to be mozilla/incoming instead of mozilla/master? Thanks! (It's okay this time.)

@catamorphism
Copy link
Contributor

Second commit is merged -- thanks!

@vertexclique
Copy link
Member Author

I have a warm welcome :) Thanks! Sorry for my fault, I will appreciate it in next commits :)

@brson
Copy link
Contributor

brson commented Oct 20, 2012

OK. I cherry-picked the valgrind change onto incoming and added a subsequent cleanup commit that addressed the CFG_VALGRIND_COMPILE issue. Thanks!

I've updated linux2's configuration to explicitly turn on valgrind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants