Conversation
…pi-to-reflect-storageproperties-api-change
…pi-to-reflect-storageproperties-api-change
shlomnissan
left a comment
shlomnissan
left a comment
There was a problem hiding this comment.
I left some questions and a couple of nits, but otherwise, LGTM
| - uses: actions/checkout@v3 | ||
| with: | ||
| submodules: true | ||
| ref: ${{ github.event.pull_request.head.sha }} |
There was a problem hiding this comment.
Which submodule do we need to check out again for this PR?
There was a problem hiding this comment.
We need to check out acquire-common. We weren't building before this, so we didn't need it.
.github/workflows/test_s3.yml
Outdated
| - name: Install | ||
| run: | | ||
| pip install --upgrade pip | ||
| pip install -e '.[testing]' |
There was a problem hiding this comment.
Why do we need to run this in "editable" mode?
|
|
||
| let dst = cmake::Config::new("acquire-common") | ||
| .profile("RelWithDebInfo") | ||
| .static_crt(true) |
There was a problem hiding this comment.
Why do we no longer link against the static version of the C runtime? How does it relate to the changes in this PR?
There was a problem hiding this comment.
This looks like an artifact from some older experimenting with the MSVC runtime. This change shouldn't be there.
| let s3_access_key_id = if value.access_key_id.nbytes == 0 { | ||
| None | ||
| } else { | ||
| Some( | ||
| unsafe { CStr::from_ptr(value.access_key_id.str_) } | ||
| .to_str()? | ||
| .to_owned(), | ||
| ) | ||
| }; | ||
| let s3_secret_access_key = if value.secret_access_key.nbytes == 0 { | ||
| None | ||
| } else { | ||
| Some( | ||
| unsafe { CStr::from_ptr(value.secret_access_key.str_) } | ||
| .to_str()? | ||
| .to_owned(), | ||
| ) | ||
| }; |
There was a problem hiding this comment.
I haven't used Rust before, so the code may be wrong, but there might be an opportunity for a utility function here to reduce duplication and isolate unsafe operations.
fn cstr_to_string(ptr: *const i8, length: usize) -> Result<Option<String>, Utf8Error> {
if length == 0 {
Ok(None)
} else {
unsafe {
let cstr = CStr::from_ptr(ptr);
let s = cstr.to_str()?.to_owned();
Ok(Some(s))
}
}
}
let s3_access_key_id = cstr_to_string(value.access_key_id.str_, value.access_key_id.nbytes)?;
let s3_secret_access_key = cstr_to_string(value.secret_access_key.str_, value.secret_access_key.nbytes)?;| // Careful: z needs to live long enough | ||
| let z = if let Some(metadata) = &value.s3_access_key_id { | ||
| Some(CString::new(metadata.as_str())?) | ||
| } else { | ||
| None | ||
| }; | ||
| let (access_key_id, bytes_of_access_key_id) = if let Some(ref z) = z { | ||
| (z.as_ptr(), z.to_bytes_with_nul().len()) | ||
| } else { | ||
| (null(), 0) | ||
| }; | ||
|
|
||
| // Careful: w needs to live long enough | ||
| let w = if let Some(metadata) = &value.s3_secret_access_key { | ||
| Some(CString::new(metadata.as_str())?) | ||
| } else { | ||
| None | ||
| }; | ||
| let (secret_access_key, bytes_of_secret_access_key) = if let Some(ref w) = w { | ||
| (w.as_ptr(), w.to_bytes_with_nul().len()) | ||
| } else { | ||
| (null(), 0) | ||
| }; |
There was a problem hiding this comment.
- I believe there's enough duplication here to warrant a utility function.
fn prepare_cstring(metadata: Option<&str>) -> Result<(*const i8, usize), std::ffi::NulError> {
match metadata {
Some(value) => {
let c_string = CString::new(value)?;
let ptr = c_string.as_ptr();
let length = c_string.to_bytes_with_nul().len();
// Note: CString is dropped here, but its pointer and length are valid
Ok((ptr, length))
}
None => Ok((null(), 0)),
}
}
let (access_key_id, bytes_of_access_key_id) = prepare_cstring(value.s3_access_key_id.as_deref())?;
let (secret_access_key, bytes_of_secret_access_key) = prepare_cstring(value.s3_secret_access_key.as_deref())?;- Also, I'm not sure what the comment
Careful: x needs to live long enoughmeans. What does it imply to not be careful? How can we write the code so that we don't need to worry about being careful?
There was a problem hiding this comment.
I don't think this would work, and I think it's related to the careful comment. If you construct a new CString in prepare_cstring, it would drop at the end of the function and the pointer would be invalid. I haven't tried to build this but I expect it would fail to compile due to Rust's lifetime restrictions.
| acquire.DeviceKind.Camera, ".*empty.*" | ||
| ) | ||
|
|
||
| # sizeof(VideoFrame) + 33 * 47 is not divisible by 8 |
There was a problem hiding this comment.
What are the implications of it not being divisible by 8? Is there an opportunity to add context to this comment?
There was a problem hiding this comment.
I'll add a docstring to this test, because it's not clear from the test itself.
tests/test_zarr.py
Outdated
| if "ZARR_S3_ENDPOINT" not in os.environ: | ||
| pytest.skip("ZARR_S3_ENDPOINT not set") | ||
| if "ZARR_S3_BUCKET_NAME" not in os.environ: | ||
| pytest.skip("ZARR_S3_BUCKET_NAME not set") | ||
| if "ZARR_S3_ACCESS_KEY_ID" not in os.environ: | ||
| pytest.skip("ZARR_S3_ACCESS_KEY_ID not set") | ||
| if "ZARR_S3_SECRET_ACCESS_KEY" not in os.environ: | ||
| pytest.skip("ZARR_S3_SECRET_ACCESS_KEY not set") |
There was a problem hiding this comment.
Nit:
required_env_vars = [
"ZARR_S3_ENDPOINT",
"ZARR_S3_BUCKET_NAME",
"ZARR_S3_ACCESS_KEY_ID",
"ZARR_S3_SECRET_ACCESS_KEY"
]
for var in required_env_vars:
if var not in os.environ:
pytest.skip(f"{var} not set")| zarr_s3_secret_access_key = os.environ["ZARR_S3_SECRET_ACCESS_KEY"] | ||
|
|
||
| dm = runtime.device_manager() | ||
| props = runtime.get_configuration() |
There was a problem hiding this comment.
Nit: Are you accessing anything else from props other than the video? Maybe we can clean some of this code by:
video = runtime.get_configuration().video[0]And then replace all props.video[0] with video.
There was a problem hiding this comment.
We use props to set the configuration later on. But we can do
props = runtime.get_configuration()
video = props.video[0]as a compromise.
aliddell
deleted the
185-update-python-api-to-reflect-storageproperties-api-change
branch
2 participants
No description provided.