Fix expand tilde and invalid path chars on windows
The crate shellexpand contained an error for windows platforms, where
tilde characters where only expanded correctly, if preceeded by a
forward slash. Although, a patch
is currently in the pipeline, the crate owner has not merged it for
almost a year and the repo generally appears inactive.
Further, the colon ':' character is disallowed in filepaths on
windows, as it is used on the NTFS filesystem to delimit alternative
data streams (e.g. %HOMEPATH%\Downloads\readme.txt:Zone.Identifier).
The violation occurs in
tor_dirmgr::src::storage::sqlite::SqliteStore::save_blob_internal
in the filename generation.
This patch therefore breaks backwards compatibility, but the given issues would prohibit windows portability as well.
The following unit tests that failed on windows are addressed by this patch:
Merge request reports
Activity
requested review from @Diziet
Hi. Thanks for the attention to portability.
However, I wonder if it might not be possible to do better than c&p the main part of shellexpand into our codebase. Options include:
- Offer to the maintainer of shellexpand to take it over
- Create a
shellexpand-fork-torcrate for our use - Temporarily use a git branch dependency pointing at the MR branch in the upstream git repo for the fix we need
I have done (1): https://github.com/netvl/shellexpand/issues/17
If it's OK with you I propose to wait a few days to see if that bears fruit.
In the meantime, would you please submit the
:filename change as a separate MR? I don't want to block that. Thanks!
Hi, thanks for reverting back!
No problem at all, I will split the MRs and resubmit the path fix by EOD.
Regarding
shellexpandI reasoned, the crate was used only in this single context. Also, it made a couple of optmizations, that would not apply in the way thattor_configuses it. I thought it might have been prefereble to just replace the crate entirely, refactoring it to cut out the unnecessary bits, reduce external dependencies, and make it generally a bit clearer to read.
-
No problem at all, I will split the MRs and resubmit the path fix by EOD.
Cool, thanks. Whenever you're able is great :-).
Regarding
shellexpandI reasoned, the crate was used only in this single context. Also, it made a couple of optmizations, that would not apply in the way thattor_configuses it. I thought it might have been prefereble to just replace the crate entirely, refactoring it to cut out the unnecessary bits, reduce external dependencies, and make it generally a bit clearer to read.Mmm. I can see that argument. However, the less code we have in our tree the less we have to maintain as part of arti. And I like to share with others. So overall I think it's better to reuse rather than import the code. Thanks for understanding.
If we want to move ahead before shellexpand upstream have had a chance to respond, I think a git dependency in our
Cargo.tomlis probably a reasonable short-term approach. @nickm, is that something we could do? -
Alright, I opened !277 (merged) to address the invalid path character in
tor_dirmgr.If we want to move ahead before shellexpand upstream have had a chance to respond, I think a git dependency in our
Cargo.tomlis probably a reasonable short-term approach.If you decide to go with this approach, I could amend this MR. Otherwise, it's probably best to close it?
Ian writes:
Mmm. I can see that argument. However, the less code we have in our tree the less we have to maintain as part of arti. And I like to share with others. So overall I think it's better to reuse rather than import the code. Thanks for understanding.
FWIW I agree with this; it would be better for the rust ecosystem to have a fixed (or forked) shellexpand than for every project that uses shellexpand to have to take a patch of this kind.
(As yet another alternative, I guess we could look to see if there are other projects that provide similar functionality. I didn't look very hard before I settled on shellexpand in the first place; as far as I know, there may already be better options.)
mentioned in merge request !277 (merged)
-
@mjptree Thanks for your patience. I think, having reread this, that our conclusion is that to fix the bugginess in shellexpand, we want to either make it a git dependency, or fork it on crates.io. @nickm didn't express an objection to a git dependency in our
Cargo.toml. So let's do that.For the avoidance of doubt, I haven't checked whether the git branch for https://github.com/netvl/shellexpand/pull/13 has the right base for us to use it. After all we might be undoing some other fixes. I think checking that is part of sorting this out.
It's only been 3 days since I volunteered to take over the shellexpand crate. I'll be away next week. If I haven't had a response by the time I get back, I will publish a fork.
-
@Diziet No worries at all. Let me know, when you are in place. Then I'll MR the bug fix for windows to the appropriate fork of
shellexpandand add a regression test to both repos.
-
I had no response from
shellexpandupstream in https://github.com/netvl/shellexpand/issues/17 . So I have forked it asshellexpand-fork, with the git repo currently in my personal gitlab.com namespace: https://gitlab.com/ijackson/rust-shellexpandI merged the change to fix Windows tilde expansion from the upstream MR into my package.
I would love an MR in my gitlab.com repo to add a test case for this. In the meantime I will prepare an MR for arti to use my fork.
-
@nickm: It is my practice to avoid being the sole owner of crates.io projects, given that crates.io has no way to reclaim a name from a sole ownr who vanishes. I think perhaps I should make you (github name
nmathewson) co-owner?
mentioned in commit Diziet/arti@72bb8e7f
mentioned in merge request !373 (merged)
added 489 commits
-
86034cc3...8231e702 - 488 commits from branch
tpo/core:main - 441de9ef - Test home and cache variable expansion
-
86034cc3...8231e702 - 488 commits from branch
-
@Diziet Yeeeeh, messed up the first attempt at pushing the tests... so, had to try a second time. Should be fine and Windows-compatible now.
Although, the addition of
shellcheckin the pre-push hooks almost threw me off. Might be worthwile to mention that somewhere in the build dependencies. Luckliy, it's available on Windows viachoco.Edited by mjptree 209 208 assert_eq!(p.path().unwrap().to_str(), expected.to_str()); 210 209 } 211 210 212 // FIXME: Add test for windows 211 #[cfg(target_family = "windows")] 212 #[test] 213 fn expand_home() { 214 let p = CfgPath::new("~\\.arti\\config".to_string()); -
Were you aware of the Rust raw strings feature? I think you could write this like this, for example:
NB, untested. Whether to do that here is a matter of taste I think, and certainly use of the doubled toothpicks is no reason not to merge this.
enabled an automatic merge when the pipeline for 206714f2 succeeds
mentioned in commit 56c2153b