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

check: Warn users with nonzero RLIMIT_CORE #23678

Merged
merged 6 commits into from
Mar 31, 2015

Conversation

richo
Copy link
Contributor

@richo richo commented Mar 24, 2015

Rationale for this, is that I lurked ulimit -c unlimited into my .profile to debug an unrelated crash, that I kept forgetting to set before hand. I then ran the test suite and discovered that I had 150 gigs of core dumps in /cores.

Very open to another approach, or to setting the limit to something higher than 0, but I think it would be nice if the build system tried to save you from yourself here.

@rust-highfive
Copy link
Collaborator

r? @brson

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

@richo
Copy link
Contributor Author

richo commented Mar 24, 2015

(The harness is a little heavyhanded because it feels like we'll probably end up with more of these sanity checks at some point, I've definitely bumped into stuff at various times that it would be nice to add a check for)

@brson
Copy link
Contributor

brson commented Mar 27, 2015

@bors: r+

@bors
Copy link
Contributor

bors commented Mar 27, 2015

📌 Commit 2fc5575 has been approved by brson

@brson
Copy link
Contributor

brson commented Mar 27, 2015

It kind of makes me uncomfortable for the build system to always be accumulating these kinds of things (which are prone to bitrot once everybody's forgotten why they were added). I approved anyway because this kind of polish does have a cumulative effect on people's impressions of Rust.

@richo
Copy link
Contributor Author

richo commented Mar 27, 2015

Sure. For what little it's worth, I'm very happy to keep working on things like this for the foreseeable future (A little while ago I went through and mostly pep8-ified src/etc/*.py). The sentiment is helpful though, for estimating what's a reasonable inclusion.

EDIT: Err, to be clear "working on" == maintaining what's already there, not just piling more ponies into the build system.

@alexcrichton
Copy link
Member

@bors: r-

Unfortunately it looks like this doesn't pass our mac bots:

cfg: version 1.0.0-dev (19bc2539c 2015-03-27) (built 2015-03-27)
cfg: build triple x86_64-apple-darwin
cfg: host triples x86_64-apple-darwin
cfg: target triples x86_64-apple-darwin
cfg: enabling more debugging (CFG_ENABLE_DEBUG)
cfg: host for x86_64-apple-darwin is x86_64
cfg: os for x86_64-apple-darwin is apple-darwin
cfg: good valgrind for x86_64-apple-darwin is 
cfg: using CC=clang (CFG_CC)
cfg: using CXX=clang++ (CFG_CXX)
cfg: enabling valgrind run-pass tests (CFG_ENABLE_VALGRIND_RPASS)
cfg: valgrind-rpass command set to "/usr/local/bin/valgrind" --error-exitcode=100 --soname-synonyms=somalloc=NONE --quiet --suppressions=/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/etc/x86.supp --suppressions=/Users/rustbuild/src/rust-buildbot/slave/auto-mac-64-opt/build/src/etc/apple-darwin.supp --tool=memcheck --leak-check=full
cfg: no xelatex found, disabling LaTeX docs
cfg: no pandoc found, omitting PDF and EPUB docs
cfg: including test rules
cfg: antlr4 not available, skipping lexer test...
The rust test suite will segfault many rustc's in the debuginfo phase.
set ALLOW_NONZERO_ULIMIT to ignore this warning
make: *** [check-sanitycheck] Error 1

@richo
Copy link
Contributor Author

richo commented Mar 27, 2015

To put all the state into this thread so it doesn't get forgotten about, I discussed with alex on irc and:

  • It might be possible to just set the allow variable on the buildbots.
  • On reflection that variable is poorly named, I will update it to ALLOW_NONZERO_RLIMIT_CORE
  • If it's not possible/reasonable to set it on the buildbox, I will update this PR to respect ALLOW_NONZERO_RLIMIT_CORE and DENY_NONZERO_RLIMIT_CORE (a hat tip to rustc's lint attrs) and make the default a warning.

@richo
Copy link
Contributor Author

richo commented Mar 28, 2015

Ok, i believe this to be fixed. Thanks for the help!

@alexcrichton
Copy link
Member

I'm going to set this opt-out environment variable on the bots and then I will r+ this, I hope to get to this soon!

@alexcrichton
Copy link
Member

@bors: r+ 4af204d

@bors
Copy link
Contributor

bors commented Mar 31, 2015

⌛ Testing commit 4af204d with merge d754722...

bors added a commit that referenced this pull request Mar 31, 2015
Rationale for this, is that I lurked `ulimit -c unlimited` into my .profile to debug an unrelated crash, that I kept forgetting to set before hand. I then ran the test suite and discovered that I had 150 gigs of core dumps in `/cores`.

Very open to another approach, or to setting the limit to something higher than 0, but I think it would be nice if the build system tried to save you from yourself here.
@bors bors merged commit 4af204d into rust-lang:master Mar 31, 2015
@richo
Copy link
Contributor Author

richo commented Mar 31, 2015

\o/ thanks!

On Tue, Mar 31, 2015 at 2:53 PM, bors notifications@github.com wrote:

Merged #23678 #23678.


Reply to this email directly or view it on GitHub
#23678 (comment).

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.

5 participants