Skip to content

refactor: remove the crate bstr from the dependency#102

Merged
jaudiger merged 2 commits intomainfrom
remove-bstr-dep
Mar 24, 2026
Merged

refactor: remove the crate bstr from the dependency#102
jaudiger merged 2 commits intomainfrom
remove-bstr-dep

Conversation

@jaudiger
Copy link
Copy Markdown
Contributor

This PR simplifies a bit the brioche-runtime-utils codebase by removing the crate bstr to instead rely exclusively on methods from std. The main advantage to use the methods from the standard is there are infaillible which removes a bunch of indirection in the code that were related to error handling (which explains the removal of 100 lines).

We still need bstr but it's now a transitive dependency.

@jaudiger jaudiger requested a review from kylewlacy January 15, 2026 19:37
@jaudiger jaudiger self-assigned this Jan 15, 2026
@jaudiger
Copy link
Copy Markdown
Contributor Author

On main:

image

With this PR:

image

@kylewlacy
Copy link
Copy Markdown
Member

I think it'll take some research on my part before I'm ready to merge this 😅

I looked at the docs for both std::ffi::OsStr and bstr on how paths and OS strings are converted to/from bytes, and I'm not clear if bstr and std are equivalent here or not. We don't support Windows (today) so that's probably not a factor, but I'm not sure how each handles e.g. a path with an invalid UTF-8 sequence. (I'm also not sure if this is a real concern today since I also don't know if we have any packages today that use non-UTF-8 paths anywhere....)

Signed-off-by: Jérémy Audiger <jeremy.audiger@icloud.com>
Copy link
Copy Markdown
Member

@kylewlacy kylewlacy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

I'm honestly still not sure about how bstr's from_bytes vs. the std library's works, and I don't really feel that strongly about keeping / removing bstr honestly (it's a nice convenience but not for how we're using it here).

I'm good to get rid of it though. It seems pretty low-risk overall, given how slowly this repo evolves and how it really gets put through its paces when brioche-packages updates it

@jaudiger let me know if you want me to take over resolving conflicts or anything here

Signed-off-by: Jérémy Audiger <jeremy.audiger@icloud.com>
@jaudiger
Copy link
Copy Markdown
Contributor Author

Thanks @kylewlacy, I manually resolved the merge conflicts, and set auto-merge.

@jaudiger jaudiger enabled auto-merge (squash) March 24, 2026 07:59
@jaudiger jaudiger merged commit 2c952cc into main Mar 24, 2026
10 checks passed
@jaudiger jaudiger deleted the remove-bstr-dep branch March 24, 2026 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants