Skip to content

More jemalloc fixes - #38675

Merged
bors merged 2 commits into
rust-lang:masterfrom
infinity0:more-jemalloc-fixes
Jan 13, 2017
Merged

More jemalloc fixes#38675
bors merged 2 commits into
rust-lang:masterfrom
infinity0:more-jemalloc-fixes

Conversation

@infinity0

@infinity0 infinity0 commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

@rust-highfive

rust-highfive commented Dec 29, 2016

Copy link
Copy Markdown
Contributor

r? @nikomatsakis

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

alexcrichton
alexcrichton reviewed Dec 29, 2016
View reviewed changes
Comment thread src/test/run-pass/allocator-default.rs Outdated

@alexcrichton alexcrichton Dec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like it's getting out of hand, perhaps you can just configure the test to only run on x86_64 linux and OSX?

@glaubitz glaubitz Jan 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the point of a testsuite if you're not running it? If users of non-x86 platforms are supposed to disable the testsuite, they shouldn't be really trusting any of the generated binaries, to be honest.

@infinity0 infinity0 Jan 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this particular case, we're simply disabling jemalloc-related tests that are not applicable on those platforms, because rust doesn't use jemalloc there. But in general, I do hope rust starts running tests on these other platforms too.

alexcrichton
alexcrichton reviewed Dec 29, 2016
View reviewed changes
Comment thread src/test/compile-fail/allocator-rust-dylib-is-jemalloc.rs Outdated

@alexcrichton alexcrichton Dec 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also seems like it's getting out of hand, can we configure this to basically only run the test on x86_64 linux and OSX as well?

@nikomatsakis

nikomatsakis commented Dec 30, 2016

Copy link
Copy Markdown
Contributor

This seems reasonable to me (modulo @alexcrichton's comment), though this code is not really my area of expertise.

@nikomatsakis

nikomatsakis commented Dec 30, 2016

Copy link
Copy Markdown
Contributor

r=me once @alexcrichton is satisfied, so:

r? @alexcrichton

@brson

brson commented Dec 30, 2016

Copy link
Copy Markdown
Contributor

Thanks for keeping these platforms running @infinity0

This was referenced Jan 12, 2017
Closed
Closed
Closed
Closed
@infinity0

infinity0 commented Jan 12, 2017

Copy link
Copy Markdown
Contributor Author

Updated to specifically enable on x86 and x86_64 only. (I did it the other way around originally to match what is done in librustc_back - which defaults to jemalloc and specifically disables it for all those other arches.) But anyway, this won't matter much once #36963 is done.

@alexcrichton

alexcrichton commented Jan 12, 2017

Copy link
Copy Markdown
Member

@bors: r+

@bors

bors commented Jan 12, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit cadebc7 has been approved by alexcrichton

@bors

bors commented Jan 13, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit cadebc7 with merge 1a2ed98...

bors added a commit that referenced this pull request Jan 13, 2017
@bors
Auto merge of #38675 - infinity0:more-jemalloc-fixes, r=alexcrichton
1a2ed98 
More jemalloc fixes

- Disable jemalloc on s390x as well (closes #38596)
- Disable jemalloc tests on platforms where it is disabled (closes #38612)
@bors

bors commented Jan 13, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing 1a2ed98 to master...

@bors
bors merged commit cadebc7 into rust-lang:master Jan 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexcrichton alexcrichton alexcrichton left review comments
+1 more reviewer
@glaubitz glaubitz glaubitz left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@alexcrichton alexcrichton

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1.14.0 tests fail (1 run-pass, 1 compile-fail) on arm64 ppc64le and s390x 1.13.0 binaries segfault when trying to build 1.14.0

7 participants

@infinity0 @rust-highfive @nikomatsakis @brson @alexcrichton @bors @glaubitz