std::os - Add join_paths, make setenv non-utf8 capable #15280
Conversation
|
You forgot to remove the |
|
@kballard Thanks, fixed. |
|
|
||
| /// Given a collection of paths, join them together into a single bytestring | ||
| /// according to the platform's conventions for the `PATH` environment | ||
| /// variable. Drops empty paths. |
There was a problem hiding this comment.
This is an excessively long summary for the function. Can you write up a shorter one-line summary first? Remember, the first line is displayed as a one-line summary in the API documentation, and the rest of the docstring is only displayed when looking at the function itself.
Similarly, split_paths() has an excessively long summary and could benefit from a rewrite there as well.
|
I don't think dropping empty paths is the correct behavior, not for |
|
Pondering on this a bit more, |
|
Note that the above applies to unix. I don't know how Windows handles empty path segments. |
|
|
||
| for path in paths.iter().map(|p| p.container_as_bytes()) { | ||
| if path.len() == 0 { continue } | ||
| if joined.len() != 0 { joined.push(sep) } |
There was a problem hiding this comment.
This would look better as
for (i, path) in paths.iter().map(|p| p.container_as_bytes()).filter(|b| !b.is_empty()).enumerate() {
if i > 0 { joined.push(sep) }(modulo the removal of empty path filtering; see my PR comments).
|
@kballard Thanks for the thorough review, as always! JFYI: Windows always includes the current working directory on the search path. But I still think you're right about dropping empty paths. |
|
If the current working directory is always included, then keeping empty paths should be harmless. The user can filter them out if they want. This also makes it more general (for use in splitting other env vars, not just |
|
@kballard good points and agreed. I'm taking the suggestion and getting rid of the filter. I'll ping you once tests pass here and I've pushed. |
|
@kballard Finally got back to this. I've updated the PR, and I believe addressed all of your comments, but please let me know if I missed anything. |
| /// | ||
| /// Returns an `Err` result if one of the input `Path`s contains an | ||
| /// invalid character for constructing the `PATH` variable (a double | ||
| /// quote on Windows or a colon on Unix). |
There was a problem hiding this comment.
Could this also mention what the &'static str returned contains? (I presume an error message?)
This commit changes `os` in three ways: * It adds a `join_paths` function that is the converse to `split_paths`, easing manipulation of the `PATH` environment variable according to platform conventions. * **Breaking change**: It changes `split_paths` to no longer drop empty paths, since they are meaningful to some shells (where they are synonymous with the current working directory). * It changes `setenv` to take a `BytesContainer` rather than a `&str` value, since environment variables may have non-utf8 values on some platforms. Since `&str` is a `BytesContainer`, this is *not* a breaking change. Along the way, it also refactors the `split_paths` function so that `cfg` switches are applied internally (and the function header is given only once). This fixes a bug: the doc comment had an example for only one platform. [breaking-change]
|
@kballard OK, I think I've fixed all the embarrassing bugs you found. See if you can find some more? |
| let n: Vec<u16> = n.utf16_units().collect(); | ||
| let n = n.append_one(0); | ||
| let v: Vec<u16> = v.utf16_units().collect(); | ||
| let v: Vec<u16> = str::from_utf8(v).unwrap().utf16_units().collect(); |
There was a problem hiding this comment.
str::from_utf8(v).unwrap() introduces failure if the input is not valid utf-8.
Perhaps setenv() needs a bool return value, with documentation stating that it may return false on Windows (and only Windows).
| in_progress.push(*b); | ||
| } | ||
| parsed.push(Path::new(in_progress)); | ||
| parsed |
There was a problem hiding this comment.
Regarding the above notes on how bash splits paths, I am curious how Windows handles the same circumstances (empty paths, paths containing just semicolons, etc).
Closes #15276 (Guide: if) Closes #15280 (std::os - Add join_paths, make setenv non-utf8 capable) Closes #15314 (Guide: functions) Closes #15327 (Simplify PatIdent to contain an Ident rather than a Path) Closes #15340 (Guide: add mutable binding section) Closes #15342 (Fix ICE with nested macro_rules!-style macros) Closes #15350 (Remove duplicated slash in install script path) Closes #15351 (correct a few spelling mistakes in the tutorial) Closes #15352 (librustc: Have the kind checker check sub-bounds in trait casts.) Closes #15359 (Fix spelling errors.) Closes #15361 (Rename set_broadast() to set_broadcast().) Closes #15366 (Simplify creating a parser from a token tree) Closes #15367 (Add examples for StrVector methods) Closes #15372 (Vec::grow should use reserve_additional, Vec::reserve should check against capacity) Closes #15373 (Fix minor issues in the documentation of libtime.)
minor: Bump rust-version to 1.70 and use the workspace one in xtask CC rust-lang/rust-analyzer#15279
This commit changes
osin two ways:join_pathsfunction that is the converse tosplit_paths,easing manipulation of the
PATHenvironment variable according toplatform conventions.
setenvto take aBytesContainerrather than a&strvalue, since environment variables may have non-utf8 values on some
platforms. Since
&stris aBytesContainer, this is not abreaking change.
Along the way, it also refactors the
split_pathsfunction so thatcfgswitches are applied internally (and the function header is givenonly once). This fixes a bug: the doc comment had an example for only
one platform.