This repository has been archived by the owner on Dec 30, 2021. It is now read-only.
Reorganize for better crate modularity #60
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
loganwendholt
changed the title
The tools_helper crate is intended to be used when developing
new reverie tool libraries. Previously, this was being made
available by adding it as a dependency, then making it public via
the reverie library crate.
However, this causes issues in certain cases when building the
binary, because the binary does not implement all the same
functions that would be present in a tool library, leading to
unresolved references.
To solve this, the tools_library is removed as a dependency and
is no longer a default member of the reverie workspace. However,
tool library projects can still include it as a dependency by adding
`tools_helper = { git = "https://github.com/iu-parfunc/reverie" }`
within `Cargo.toml`
|
@wangbj is it normal for these two tests to report a failure? Running |
|
the master branch requires a kernel with patch from |
|
My main dev machine is running Ubuntu 18.04 with kernel |
|
If you're not using X/Wayland then it should be fine. for some reason it prevents ubuntu launch window manager, though I suspect it has nothing to do with the patch itself. |
|
@wangbj what kernel version is the patch expecting? I tried to apply it to the kernel source from |
|
I tried on both 5.2 & 5.3 (rc) and both successed |
|
@wangbj I applied the patch to the 5.3 kernel, and when I run the Does running locally pass all the tests on your machine? |
|
Do you have the full output, also would it always fail for |
|
Yes, |
wangbj
reviewed
Aug 27, 2019
There was a problem hiding this comment.
More consistent naming for sub crates.
Thanks for submitting the PR, the naming (before the PR) was poor, compare to create like: https://github.com/rust-lang-nursery/futures-rs. Can you help converting the sub crate naming more consistent names?
Also there's a lot of exported (pub) symbols in ffi.rs, are they necessary?
tools_helper/src/ffi.rs
Outdated
| } | ||
|
|
||
| #[no_mangle] | ||
| unsafe extern "C" fn syscall_hook_trampoline() { | ||
| pub unsafe extern "C" fn syscall_hook_trampoline() { |
There was a problem hiding this comment.
Lots of functions are changed to pub, are they necessary?
There was a problem hiding this comment.
I thought they would be, since ffi.rs was essentially being hidden inside a new crate rather than being copied in as a source file. However, I have to admit I don't fully understand exactly how all of these trampoline functions are being used.
I just tried taking out all the extra pubs, built the project again, and ran cargo test. Didn't see any errors. Also ran the tool with fingerprinter, and saw expected output there as well. So as far as I can tell, these pubs are not necessary.
I just pushed a new commit to address this.
There was a problem hiding this comment.
Thanks, these wrappers existed just to make sure the _trampoline* symbols didn't get stripped away during linking stage. without the wrappers the linker would have thought nobody needs them.
There was a problem hiding this comment.
No problem. I'll go ahead and reorganize the sub-crates similar to futures-rs as well.
|
@wangbj does this latest commit achieve what you were looking for in terms of a more consistent naming scheme? |
Is it possible to keep |
|
|
|
yes, that's correct. |
|
I think those changes all sound fine, so I went ahead and implemented them. How does this new commit look? |
|
Thanks a lot! |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR intends to remove the need for source files to be copied into various crates within the workspace via build scripts by consolidating these commonly used files into a new crate called
common. Crates which need these files can then simply add a dependency oncommonto theirCargo.tomlfile.In addition, this PR makes changes to the
tool_librarycrate and thereverielibrary crate which allows tool library projects to add a dependency on thereverielibrary crate, rather than copyingffi.rsandconsts.rsdirectly.