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

Fix string copy test by using predictable struct memory layout for AllocatedPyASCIIObject #574

Merged
merged 5 commits into from
May 23, 2023

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented May 14, 2023

Resolves failing test test_copy_string:

---- python_data_access::tests::test_copy_string stdout ----
copied: "\0\0\0\0\0\0\0\0\0\0\0\0\0"
thread 'python_data_access::tests::test_copy_string' panicked at 'assertion failed: `(left == right)`
  left: `"\0\0\0\0\0\0\0\0\0\0\0\0\0"`,
 right: `"function_name"`', src/python_data_access.rs:490:9

or

 ---- python_data_access::tests::test_copy_string stdout ----
thread 'python_data_access::tests::test_copy_string' panicked at 'called `Result::unwrap()`
 on an `Err` value: invalid utf-8 sequence of 1 bytes from index 2', src/python_data_access.rs:370:58

which occurs due to a failure to copy test data during into a AllocatedPyASCIIObject during to_asciiobject due to optimization of the struct layout by Rust. We can ask for a C-compatible layout to get a consistent layout for pointer arithmetic.

See passing builds at zanieb#3
See failing builds at zanieb#1 or #541

@zanieb zanieb force-pushed the fix-test branch 2 times, most recently from dd8b6d2 to 04858e8 Compare May 15, 2023 16:08
zanieb added 2 commits May 15, 2023 14:52
Requires self-hosted runners; todo to build these on standard runners
@zanieb
Copy link
Contributor Author

zanieb commented May 18, 2023

Failing freebsd build addressed in #576

@benfred
Copy link
Owner

benfred commented May 23, 2023

thanks for the fix! it looks good - aside from that I'd like to maintain testing on ARM here .

@benfred benfred merged commit e6053b5 into benfred:master May 23, 2023
@zanieb
Copy link
Contributor Author

zanieb commented May 23, 2023

thanks for the fix! it looks good - aside from that I'd like to maintain testing on ARM here .

Ah sorry about that looks like I didn't do a great job cherry-picking it back over here

@benfred
Copy link
Owner

benfred commented May 23, 2023

no worries! thanks again for the fix -

@benfred benfred added the bug Something isn't working label Aug 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants