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

rust: split crates into libceed-sys and libceed #766

Merged
merged 18 commits into from
May 28, 2021
Merged

Conversation

jedbrown
Copy link
Member

@jedbrown jedbrown commented May 20, 2021

  • split crates
  • document libceed-sys/src/lib.rs (briefly)
  • update docs for versioned packages from crates.io
  • set include/exclude keys for cargo package
  • fix failing Rust CI
  • dry-run publish

Co-authored-by: Jeremy L. Thompson <jeremy@jeremylt.org>
@jedbrown jedbrown requested a review from jeremylt May 20, 2021 21:52
@jedbrown jedbrown added the Rust label May 20, 2021
@jeremylt jeremylt added this to the v0.8.1 milestone May 20, 2021
@jeremylt
Copy link
Member

jeremylt commented May 20, 2021

This issue indicates that packaging with symlinks should maybe work, but currently it does not actually work for us.

The entire c-src symlink is completely ignored without warning.

jeremy@Sinensis:~/Dev/libCEED/rust/libceed-sys$ cargo package --list
.cargo_vcs_info.json
Cargo.toml
Cargo.toml.orig
build.rs
src/lib.rs

@jedbrown
Copy link
Member Author

Should be fixed now. The issue seems to have been discovering a Cargo.toml in that subdirectory.

@jeremylt
Copy link
Member

Ah. Annoying.

@jeremylt
Copy link
Member

The CI failure is with detection of Valgrind

  /home/runner/work/libCEED/libCEED/rust/libceed-sys/c-src/backends/memcheck/ceed-memcheck-qfunction.c:19:10: fatal error: 'valgrind/memcheck.h' file not found
  #include <valgrind/memcheck.h>
           ^~~~~~~~~~~~~~~~~~~~~
  1 error generated.
  make: *** [Makefile:489: /home/runner/work/libCEED/libCEED/target/debug/build/libceed-sys-d1ea22c6e1ad3f23/out/build/backends/memcheck/ceed-memcheck-qfunction.o] Error 1

Investigating

@jeremylt
Copy link
Member

Ok, CI should be happy now and cargo package --dry-run works for libceed-sys. We'll need to first publish libceed-sys before we can get the dry run to work for the libceed crate though.

@jedbrown
Copy link
Member Author

Anyone know off-hand how to make a README.md be used by docs.rs?

@YohannDudouit
Copy link
Contributor

[package]
readme = "/path/to/readme/README.md"

?

@jedbrown
Copy link
Member Author

jedbrown commented May 28, 2021

Well, we currently use rST and even if we switch to md, it'll likely be the MyST dialect. But we currently have crate docs in src/lib.rs and I mildly prefer it there because we can test code snippets, while as far as I know, code snippets in README.md are not tested.

So I was thinking if docs.rs could put the README.md at the top, followed by the more detailed docs in src/lib.rs, we'd have an okay solution that didn't duplicate text. Is that what your suggestion does, or use it just being explicit about where crates.io should get its readme?

This preserves the development mode in which you can put flags in
c-src/config.mk and have them used in your build.
@jedbrown
Copy link
Member Author

Closest I've found seems to be cargo-readme, which writes README.md from the src/lib.rs rustdoc comments. What do y'all think? Should we use it? (It becomes a release step because cargo doesn't do it automatically.)

The explicit lib section without explicit path was breaking
cargo-readme.
@jeremylt
Copy link
Member

Another possible use of #[cfg(doctest)] is to test doctests that are included in your README file without including it in your main documentation. For example, you could write this into your lib.rs to test your README as part of your doctests:

#![feature(external_doc)]

#[doc(include = "../README.md")]
#[cfg(doctest)]
pub struct ReadmeDoctests;

This will include your README as documentation on the hidden struct ReadmeDoctests, which will then be tested alongside the rest of your doctests.

What about this?

@jeremylt
Copy link
Member

Here we go, found the page on it
https://doc.rust-lang.org/unstable-book/language-features/external-doc.html
It is an unstable feature and it's not clear to me if the documentation at the top of the lib file can be included in this way. We can tinker tomorrow though

@jedbrown
Copy link
Member Author

Good find, and it works for module documentation, but from what I can tell, not for crate-level docs.

@jeremylt
Copy link
Member

rust-lang/rust#83366

This might also be an option

@jedbrown
Copy link
Member Author

I got it working, though I had to read this section a half-dozen times to catch the distinction between #[doc = ...] and #![doc = ...].

@jedbrown
Copy link
Member Author

🎉 https://crates.io/crates/libceed

This PR is ready from my perspective. It's published as 0.8.0 (nominally distinct from libCEED C library version) and we can iron out any quirks before cutting 0.8.1 (and encouraging external users of the Rust interace). I will add notes on release procedure to #725 .

Cc: @ZackJorquera

Copy link
Member

@jeremylt jeremylt left a comment

Choose a reason for hiding this comment

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

Looks ready to me

@jedbrown jedbrown merged commit c4016ce into main May 28, 2021
@jedbrown jedbrown deleted the jed/rust-crates branch May 28, 2021 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants