-
Notifications
You must be signed in to change notification settings - Fork 14
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
Test failures on Windows #678
Comments
Of note, I updated my local Rust version to 1.84 on my Mac and cannot reproduce the failures. So it has something to do with 1.84 and probably the way longjmps work on Windows? |
Look into rust-lang/rust#123470 because the backtrace looks similar, are we getting some double panic scenario now that we could catch? |
* Analyze completion site re: trailing parens * Start dismantling measures that just aided development * More parallel with the other branch * Get inspired by the less nested approach in PR #369 * Must have been leftover from an experiment * Add tests * Be consistent * Try to get a bit more info on the Windows test failure * Flatten this out too * Not my problem to solve: Revert "Try to get a bit more info on the Windows test failure" This reverts commit dea921d. This is unrelated to this PR and is being tracked in #678. * Remove rest of temporary logging * Extract out `node_find_containing_call()` * Rework on top of `parameter_hints()` idea * Add `ParameterHints` enum And fix a bug with imports environment functions not respecting `parameter_hints` * Move tests to `parameter_hints.rs` It seems obvious to me that we are testing a feature that "officially" lives here now * Tweak examples probed in tests * Add comment * Use original name of function --------- Co-authored-by: Davis Vaughan <[email protected]>
I have made some serious progress here static MSG: Lazy<CString> = Lazy::new( || CString::new("ouch").unwrap());
pub fn top_level_exec2() -> harp::Result<()>
{
extern "C" fn callback(_args: *mut c_void)
{
// let msg = CString::new("ouch").unwrap();
unsafe { Rf_error(MSG.as_ptr()) };
}
unsafe { R_ToplevelExec(Some(callback), std::ptr::null_mut()) };
Ok(())
} #[test]
fn test_top_level_exec() {
crate::r_task(|| {
let _ = top_level_exec2();
})
} With this minimal reprex, you do NOT get a panic when using I believe this means we are right-ish with our thinking that it has to do with Drop handling. The Still not sure why or how we fix it, but getting down to this reprex of crash vs no crash is big |
Ooooh ok I don't understand the full implications of this yet but changing from |
That's right, this top-level-exec is only a safeguard. It's still important to run code in the safest way possible inside the callback. |
Note that the specific test you see there is a red herring and that's just the first of many tests that are failing. Here is a more informative bit of output from nextest from some testing in #676:
@lionel- and I believe this is an issue on Windows that has cropped up between Rust 1.83 and 1.84, in particular we think it is related to our
try_catch()
implementation and rust-lang/rust#129582. It is failing all of a sudden because GitHub Actions just updated their Windows runner to 1.84It is hypothesized that the
closure
that calls C level R code is now getting dropped when R longjmps, causing things inside that closure to now also get dropped, and possibly that is causing issues all of a sudden. It is quite hard to tell at the moment.The text was updated successfully, but these errors were encountered: