Add S3 support

[aliddell]

No description provided.

[shlomnissan]

I left some questions and a couple of nits, but otherwise, LGTM

[shlomnissan]

Which submodule do we need to check out again for this PR?

[aliddell]

We need to check out acquire-common. We weren't building before this, so we didn't need it.

[shlomnissan]

Why do we need to run this in "editable" mode?

[aliddell]

We probably don't.

[shlomnissan]

Why do we no longer link against the static version of the C runtime? How does it relate to the changes in this PR?

[aliddell]

This looks like an artifact from some older experimenting with the MSVC runtime. This change shouldn't be there.

[shlomnissan]

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)?;

[shlomnissan]

• 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 enough means. 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?

[aliddell]

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.

[shlomnissan]

What are the implications of it not being divisible by 8? Is there an opportunity to add context to this comment?

[aliddell]

I'll add a docstring to this test, because it's not clear from the test itself.

[shlomnissan]

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")

[shlomnissan]

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.

[aliddell]

We use props to set the configuration later on. But we can do

props = runtime.get_configuration()
video = props.video[0]

as a compromise.