Skip to content

trpl: ffi: clean up examples and push crates.io #29790

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 4 commits into from
Closed

trpl: ffi: clean up examples and push crates.io #29790

wants to merge 4 commits into from

Conversation

durka
Copy link
Contributor

@durka durka commented Nov 12, 2015

Crates.io is now mentioned right at the top to try and head off the #![feature(libc)] error.

In addition, the examples now all compile and run in the playpen. I also tweaked spacing in some of them.

Fixes #29762.

r? @steveklabnik

@durka durka force-pushed the trpl-ffi-cratesio branch from c1dcf87 to 566330b Compare November 12, 2015 00:08
steveklabnik
steveklabnik reviewed Nov 12, 2015
View reviewed changes
src/doc/trpl/ffi.md
@@ -85,8 +101,11 @@ the allocated memory. The length is less than or equal to the capacity.
# #![feature(libc)]
# extern crate libc;
# use libc::{c_int, size_t};
#
Copy link
Member

Choose a reason for hiding this comment

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

we don't usually add extra lines like this here, as they're not displayed anyway, and just take up space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I can take the spaces back out.

@steveklabnik
Copy link
Member

Why are these cfgs being added?

@durka
Copy link
Contributor Author

durka commented Nov 12, 2015

I add the cfg business to make the examples compile in the playpen. If that stuff stays I guess they can also be changed from no_run to rust. But maybe it's too much for stuff that isn't displayed unless you click through.

durka
durka reviewed Nov 12, 2015
View reviewed changes
src/doc/trpl/ffi.md
@@ -16,11 +27,14 @@ compile if snappy is installed:
extern crate libc;
use libc::size_t;

#[cfg(nope)]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, I left out the # here.

@durka
Copy link
Contributor Author

durka commented Nov 12, 2015

I just realized the cfg hack is way unnecessary, especially the #[cfg(not(nope))] because hidden lines are hidden. Instead we can just use # /* .. # */ to comment out the extern-bits for the playpen, and just hide the shim lines from the book, with no cfg.

@durka
Copy link
Contributor Author

durka commented Nov 12, 2015

Looks better now (to me).

@steveklabnik
Copy link
Member

@bors: r+ rollup

@bors
Copy link
Collaborator

bors commented Nov 12, 2015

📌 Commit 9454fd4 has been approved by steveklabnik

@steveklabnik
Copy link
Member

Thanks!

steveklabnik added a commit to steveklabnik/rust that referenced this pull request Nov 12, 2015
@steveklabnik
Rollup merge of rust-lang#29790 - durka:trpl-ffi-cratesio, r=stevekla…
3026d34 
…bnik

Crates.io is now mentioned right at the top to try and head off the `#![feature(libc)]` error.

In addition, the examples now all compile and run in the playpen. I also tweaked spacing in some of them.

Fixes rust-lang#29762.

r? @steveklabnik
@steveklabnik steveklabnik mentioned this pull request Nov 12, 2015
Closed
@bors
Copy link
Collaborator

bors commented Nov 13, 2015

⌛ Testing commit 9454fd4 with merge 517d23c...

@bors
Copy link
Collaborator

bors commented Nov 13, 2015

💔 Test failed - auto-linux-64-nopt-t

@steveklabnik
Copy link
Member

Looks like a legit failure here.

@durka
Copy link
Contributor Author

durka commented Nov 16, 2015

Ah yep, readline is installed in the playpen but not on the buildbot.

@steveklabnik
Copy link
Member

@edunham @brson what do we want to do here? should we install readline so that we can have these compiled, or just leave them ignored?

@alexcrichton
Copy link
Member

Just be sure to tag the tests with no_run or something that doesn't try to actually link them, we don't want to run them I believe.

@durka durka closed this Nov 16, 2015
@durka
Copy link
Contributor Author

durka commented Nov 16, 2015

The point was to not need no_run but if that's not a goal, no point. Closing.

@brson
Copy link
Contributor

brson commented Nov 16, 2015

Thanks @durka.

I agree that we can't remove no_run here, sadly, since we need the test suite to pass on all supported platforms. This is a common problem with FFI tests and it would be nice if we had a more robust solution

@durka
Copy link
Contributor Author

durka commented Nov 16, 2015

Well, you can remove them by stubbing out the FFI functions and hiding the stub code from rustdoc, but then part of the code in the example isn't tested, so it's not a perfect solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@durka @steveklabnik @bors @alexcrichton @brson