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

configure: Disable rpass-valgrind when there's no valgrind #24859

Merged
merged 4 commits into from
May 9, 2015

Conversation

richo
Copy link
Contributor

@richo richo commented Apr 27, 2015

This stung me more than once in dev.

Bonus DRY'ing up of configure that I did on my way past.

@rust-highfive
Copy link
Collaborator

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member

Hm I'm a little hesitant to do this as it will silently stop running all valgrind tests on the bots if we reimage and forget to install valgrind.

@brson
Copy link
Contributor

brson commented Apr 28, 2015

I have another patch that makes configure error if valgrind isn't available: #24477. Personally, I do not like requiring valgrind to configure and build and might be in favor of this.

@richo
Copy link
Contributor Author

richo commented Apr 28, 2015

I'd be in favour of tweaking this patch to make configure error if you pass --enable-valgrind and it can't be found. @alexcrichton is it plausible to just ensure that the builders pass that flag when they're configuring?

@alexcrichton
Copy link
Member

@richo that seems reasonable, but it also seems like #24477 does that?

@richo
Copy link
Contributor Author

richo commented Apr 28, 2015

It looks like #24477 will fail by default if valgrind isn't present, which I don't believe to be the behaviour we want to expose to end users. I'll probably update this patch tonight but I don't have a particular horse in this race, eg I'd be perfectly content with an updated version of @brson's that won't break by default without valgrind.

@richo
Copy link
Contributor Author

richo commented Apr 28, 2015

I updated the branch. It'll now fail if you do something like:

./configure --enable-valgrind

and don't have a valgrind, otherwise it will just disable the valgrind rpass tests

@alexcrichton
Copy link
Member

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned alexcrichton Apr 28, 2015
@pnkfelix
Copy link
Member

@alexcrichton Is the idea that we first land this, and then after it is landed, we update the bots ASAP to pass --enable-valgrind ?

Or do we already pass --enable-valgrind on the bots?

@alexcrichton
Copy link
Member

Looks like we're currently passing --disable-valgrind, so yes we'd have to change the bots.

@richo
Copy link
Contributor Author

richo commented May 5, 2015

ping @brson

@brson
Copy link
Contributor

brson commented May 8, 2015

OK, so as I understand it this patch turns on valgrind-rpass by default if valgrind exists, but otherwise silently disables it. If --enable-valgrind is passed explicitly and valgrind does not exist then this emits a new error.

@richo I'm sorry for the delay and hate to ask you to do more, but can you add a warning message to mk/main.mk saying that valgrind-rpass was disabled, similar to the existing case where it prints a status message when it is enabled. Since this case causes the build to miss some test coverage it's important to point that out I think.

@brson
Copy link
Contributor

brson commented May 8, 2015

r=me after that nit is addressed

@richo
Copy link
Contributor Author

richo commented May 8, 2015

Sorry for delay, added the log message.

r? @brson

@brson
Copy link
Contributor

brson commented May 8, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented May 8, 2015

📌 Commit 01fc026 has been approved by brson

@brson
Copy link
Contributor

brson commented May 8, 2015

@alexcrichton @pnkfelix What do you anticipate changing on the bots when this patch lands? I don't see it (maybe I don't actually understand this patch...). I think all that will happen is this will run on machines w/o valgrind and disable the valgrind-rpass tests.

On closer inspection I don't understand how the valgrind-rpass code works.

@richo
Copy link
Contributor Author

richo commented May 8, 2015

Err, wait yeah I think I might have goofed at some point in a rebase. Sorry this pr is weeks old and I don't really remember how it works either. Want to r- and I'll have a closer peek today? Apologies!

@richo
Copy link
Contributor Author

richo commented May 8, 2015

EDIT: No, it does the correct thing (CFG_VALGRIND is populated by probing for a valgrind binary on PATH)

So the only change to the bots should be to pass --enable-valgrind explicitly, which will make reimaging them without installing valgrind a configure error, and not silently ignore those tests.

@alexcrichton
Copy link
Member

@brson I think we'd want to pass --enable-valgrind on the machines where we expect rpass-valgrind to run (so it's a configure error if we forget to install valgrind). I'd have to check though to make sure that --enable-valgrind only enables it for tests.

bors added a commit that referenced this pull request May 9, 2015
This stung me more than once in dev.

Bonus DRY'ing up of configure that I did on my way past.
@bors
Copy link
Contributor

bors commented May 9, 2015

⌛ Testing commit 01fc026 with merge 8c9dc18...

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.

6 participants