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

run-pass-valgrind tests fail when valgrind doesn't exist #18588

Closed
brson opened this issue Nov 3, 2014 · 9 comments
Closed

run-pass-valgrind tests fail when valgrind doesn't exist #18588

brson opened this issue Nov 3, 2014 · 9 comments
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc

Comments

@brson
Copy link
Contributor

brson commented Nov 3, 2014

I get task '<main>' panicked at 'Can't find Valgrind to run Valgrind tests', /opt/dev/rust3/src/compiletest/compiletest.rs:44 when running the test suite now. These tests should probably be disabled when configure doesn't find valgrind.

@brson brson added the A-testsuite Area: The testsuite used to check the correctness of rustc label Nov 3, 2014
@brson
Copy link
Contributor Author

brson commented Nov 3, 2014

cc @nick29581

@nrc
Copy link
Member

nrc commented Nov 3, 2014

cc @alexcrichton, this is deliberate.

You can avoid it by using the flag --disable-valgrind-rpass

@brson
Copy link
Contributor Author

brson commented Nov 3, 2014

If it is on purpose then the configure script should have failed when valgrind wasn't found.

@liigo
Copy link
Contributor

liigo commented Nov 4, 2014

I got the same error, and didn't know how to do, before I'm here and find --disable-valgrind-rpass.

@liigo
Copy link
Contributor

liigo commented Nov 4, 2014

We need a more friendly thing, like what @brson suggested.

@ghost
Copy link

ghost commented Jan 23, 2015

An easy fix(unless I'm missing something, of course) would be adding the the following lines after this one:

rust/configure

Line 642 in 81504f2

probe CFG_VALGRIND valgrind

Looks like this(first line already existing):

probe CFG_VALGRIND         valgrind
if [ -z "$CFG_VALGRIND" ]
then
    if [ ! -z "$CFG_ENABLE_VALGRIND" ] || [ -z "$CFG_DISABLE_VALGRIND_RPASS" ]
    then
      err "Valgrind not found, but wanted. You may want to add --disable-valgrind-rpass"                      
    fi
fi

This will yell by default(./configure without params) if valgrind isn't found, unless you specifically add --disable-valgrind-rpass.

However, the reason why I didn't make a PR is because this code won't work in the case that the default values will change. I am speaking of these values from here:

rust/configure

Line 501 in 81504f2

opt valgrind 0 "run tests with valgrind (memcheck by default)"

reproduced here, these values I meant(0,0,1):

opt valgrind 0 "run tests with valgrind (memcheck by default)"
opt helgrind 0 "run tests with helgrind instead of memcheck"
opt valgrind-rpass 1 "run rpass-valgrind tests with valgrind"

adding this code within the if [ -z "$CFG_VALGRIND" ] will show what I mean when you test specific --enable and --disable parameters:

  echo "~$CFG_ENABLE_VALGRIND~$CFG_ENABLE_VALGRIND_RPASS~$CFG_DISABLE_VALGRIND_RPASS~$CFG_DISABLE_VALGRIND"

Basically, if the default value for say valgrind is 0, then --enable-valgrind will set $CFG_ENABLE_VALGRIND but, passing --disable-valgrind will not set $CFG_DISABLE_VALGRIND.
So, if anyone will change that default valgrind value to 1, or more likely the default value valgrind-rpass from 1 to 0 then the above code will have to be changed from [ -z "$CFG_DISABLE_VALGRIND_RPASS" ] to [ ! -z "$CFG_ENABLE_VALGRIND_RPASS" ].

I figure this is done this way to detect if the value was specifically requested/set by the user(via a ./configure parameter), instead of the default one(./configure without params).
So no variable will be set($CFG_ENABLE_X / $CFG_DISABLE_X) unless the user specifically requested it via ./configure --enable_X or ./configure --disable_X .

@brson
Copy link
Contributor Author

brson commented Apr 15, 2015

@emanueLczirai the problem you've discovered is an unfortunate limitation of the configure system. Writing conditionals about configuration correctly requires knowing the default value.

brson added a commit to brson/rust that referenced this issue Apr 15, 2015
@tamird
Copy link
Contributor

tamird commented Nov 8, 2015

Looks like this should have been closed by #24859. @brson?

@alexcrichton
Copy link
Member

Indeed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc
Projects
None yet
Development

No branches or pull requests

5 participants