-
Notifications
You must be signed in to change notification settings - Fork 1.1k
ctest: Add translation of Rust types. #4501
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
base: main
Are you sure you want to change the base?
Conversation
There's quite a lot of |
fc9641c
to
1d766de
Compare
Tests seem to fail at a parse int error, so |
43a06e3
to
3c411d1
Compare
The error on msvc goes away if I replace |
From what I understand |
6d9706f
to
030ce77
Compare
030ce77
to
4241857
Compare
ctest-next/build.rs
Outdated
.or_else(|_| env::var("CC")) | ||
.or_else(|_| env::var(format!("CC_{target_key}"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For linker, the env is LD
. CC
is just the compiler (don't you need that too though?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't the compiler also act as the linker if given the correct arguments? I didn't think of it that much but passing whatever is set for those three variables as the linker worked (and in the CI it's the compiler not the linker).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often they are but not necessarily, that's the difference between the CC
and LD
variables. The CARGO_*_LINKER
env doesn't actually need to build anything (it's only used for rustc
's final link) so somebody could pass ld
rather than gcc
. At least, that should work but the flags do need to line up.
So you should just reexport all variables starting with CC
for the cc
crate (probably just iterate the env and export everything starting with CC
) and then use the CARGO_*_LINKER
var to the rustc
invocation with -Clinker=...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, in our cases we use the compiler for both. But we don't want it to blow up if this isn't the case of course, so we shouldn't assume that they will always be the same.
That snip actually has a good point though, the AR_*
variables also need to be forwarded.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would they need to be? The ENV vars are set before it runs and they are not unset by cc
. TARGET
and HOST
are only set for the duration of build.rs so they have to be forwarded. The linker and runner are forwarded there for convenience.
From it's docs:
"In addition to the above optional environment variables, cc-rs has some functions with hard requirements on some variables supplied by cargo’s build-script driver that it has the TARGET, OUT_DIR, OPT_LEVEL, and HOST variables."
So those 4 are the only ones that really have to be forwarded, linker and runner are just for convenience. In one of the previous CI runs for the powerpc64 architecture, I remember the CC_target variable was set in the debug output of cc
despite ctest
not setting it because it was already set by ENV
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough, you're right that we don't actually need to manually do all of these. Anything CARGO_*
does need to be extracted because we need to pass it to rustc
(for linking) or the runner (for qemu) since we're not going via Cargo. But the rest we can just let cc
figure out.
ctest-next/src/generator.rs
Outdated
} | ||
|
||
/// Configures the host. | ||
pub fn host(&mut self, host: &str) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't in the original ctest
; why is it needed here? cc
should be able to figure out the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before ctest only worked inside of build.rs
which set the HOST
env var for us, but ctest-next
forwards those from the build.rs
instead so it has to be manually set for cc inside generate
. I thought that it would be better to also have the option to manually set it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this isn't used for tests or anything currently, so let's just remove it for now. Easy to add back once/if we need it.
ctest-next/tests/basic.rs
Outdated
gen.header("simple.h") | ||
.generate(crate_path, "simple_out") | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.header
should be an included header and not an output, right? Where is simple.h
coming from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: we should be building a simple.c
file (using the template) and asserting it has the same contents as tests/input/simple.c
. Or, if CTEST_BLESS=1
is present, update the file instead. That way we see what the generated output looks like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But isn't that different from the old ctest
? Here it already has a manually written header file for t1.c
, it uses t1.rs
to generate t1gen.c
and t1gen.rs
, t1gen.c
includes the header file t1.h
and uses it to define test functions we call in t1gen.rs
via ffi. Then finally it would build bin/t1.rs
that includes the original t1.rs
as well as the generated one, to run the tests.
So the generated C file just holds tests to be ran and uses the header file to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, you are correct. In that case we should have both t1.h
which is handwritten, and t1.c
which is the autogenerated file that gets updated if CTEST_BLESS
is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^ I forgot we also build a Rust file; that should get blessed as well. I guess if the test file is basic.rs
, name the output file basic.out.rs
. Maybe also basic.out.c
rather than basic.c
for some consistency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to ensure that we also reject fat pointer types like &str
and &[T]
ctest-next/tests/basic.rs
Outdated
gen.header("simple.h") | ||
.generate(crate_path, "simple_out") | ||
.unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify: we should be building a simple.c
file (using the template) and asserting it has the same contents as tests/input/simple.c
. Or, if CTEST_BLESS=1
is present, update the file instead. That way we see what the generated output looks like.
Have you been able to run any cross build+test locally by the way? Thinking about things a bit more, I'm not sure things will work with the current setup (building in integration tests). The biggest problem is that on cross compiled targets it will be trying to run host tools from within the runner: e.g. when you're running an integration test in Unfortunately I think this might mean we have to use the original For this PR: we need to have the "bless" tests that show what the generated |
I was able to get powerpc64 to cross compile from x86_64 linux (such that only the Running the test doesn't invoke It should be possible, but I do agree it would be somewhat hacky, making sure that specific environment variables are set and forwarded. |
Doesn't it? The |
Oh wait, you're right. It wasn't a problem when I tried cross compiling powerpc64, it probably does use the host provided cc and rustc, but so long as we specify the correct linker and such with the environment variables it won't fail. |
7a675b9
to
721e13e
Compare
I'm not very familiar with how Rust syntax should be translated to C, so the unit tests were made according to my understanding, if the translation is wrong I'll change the parser. Similarly I made a bunch of changes to |
ctest-next/src/generator.rs
Outdated
} | ||
|
||
/// Configures the host. | ||
pub fn host(&mut self, host: &str) -> &mut Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems like this isn't used for tests or anything currently, so let's just remove it for now. Easy to add back once/if we need it.
/// Generate the Rust and C testing files. | ||
/// | ||
/// Returns the path to t generated file. | ||
pub fn generate_files( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For each generated file, you should do a regex to replace \n{3,}
with \n\n
to get rid of all the extra whitespace that the templates seem to insert.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, it looks like Askama has #[template(whitespace = "suppress")]
or #[template(whitespace = "minimize")]
. Might be worth playing with before the regex thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few remaining small comments, then I think we can get this merged!
Also please rebase + clean up history.
[build] | ||
rustflags = ["--cfg", "procmacro2_semver_exempt"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is kind of crummy isn't it. .cargo/config.toml
is kind of unreliable since it doesn't set config for anything that uses this as a dependency, I was thinking this was a Cargo feature.
So for now, could you just comment out the .file()
.start()
etc parts of the message with a FIXME(ctest):...
? These just stabilized so we should be able to use them somewhat soon dtolnay/proc-macro2#503.
ctest-next/src/tests.rs
Outdated
.collect::<Vec<_>>(), | ||
["malloc"] | ||
translator.translate_type(&ty), | ||
Ok("uint8_t (*const *) [5]".to_string()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can just figure this out once we have better testing with C. For now, just comment out the test with a FIXME
let new_content = fs::read_to_string(&new_file)? | ||
.replace("\r\n", "\n") | ||
.replace("\r", ""); | ||
let old_content = fs::read_to_string(&old_file)? | ||
.replace("\r\n", "\n") | ||
.replace("\r", ""); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to remove the first replace for each file, since the second replace does the same thing.
.replace("\r", ""); | ||
|
||
let equal = new_content == old_content; | ||
if env::var("LIBCBLESS").is_ok() && !equal { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if env::var("LIBCBLESS").is_ok() && !equal { | |
if env::var("LIBC_BLESS").is_ok() && !equal { |
/// | ||
/// If the contents do not match and LIBCBLESS is set, overwrite the | ||
/// test file with the content of the generated file. | ||
fn bless_equal(new_file: impl AsRef<Path>, old_file: impl AsRef<Path>) -> Result<bool> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to have a Result
here I think, you can just do the assertions in this function. Use https://docs.rs/pretty_assertions/latest/pretty_assertions/ so there is a nice diff.
use super::*; | ||
|
||
use std::mem; | ||
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; | ||
use std::fmt::{Debug, LowerHex}; | ||
use std::ptr; | ||
use std::slice; | ||
use std::ffi::CStr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use super::*; | |
use std::mem; | |
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; | |
use std::fmt::{Debug, LowerHex}; | |
use std::ptr; | |
use std::slice; | |
use std::ffi::CStr; | |
use std::ffi::CStr; | |
use std::fmt::{Debug, LowerHex}; | |
use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; | |
use std::{mem, ptr, slice}; | |
use super::*; |
Making this match rustfmt
if FAILED.load( std::sync::atomic::Ordering::Relaxed) { | ||
panic!("some tests failed"); | ||
} else { | ||
println!("PASSED {} tests", NTESTS.load( std::sync::atomic::Ordering::Relaxed)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if FAILED.load( std::sync::atomic::Ordering::Relaxed) { | |
panic!("some tests failed"); | |
} else { | |
println!("PASSED {} tests", NTESTS.load( std::sync::atomic::Ordering::Relaxed)); | |
} | |
if FAILED.load(std::sync::atomic::Ordering::Relaxed) { | |
panic!("some tests failed"); | |
} else { | |
println!( | |
"PASSED {} tests", | |
NTESTS.load(std::sync::atomic::Ordering::Relaxed) | |
); | |
} |
Same thing, just making rustfmt happy with the output files
Description
The ability to translate various Rust types that are translatable into equivalent C types. Additionally some basic unit tests have been added.
Sources
Checklist
libc-test/semver
have been updated*LAST
or*MAX
areincluded (see #3131)
cd libc-test && cargo test --target mytarget
);especially relevant for platforms that may not be checked in CI