Add support for '~otheruser/directory' expansion on *nix systems

[bcmyers]

Cool crate. I was going to building something similar myself when I came across your library.

I noticed that at the moment you're not able to handle ~otheruser/directory expansions. This pull request adds that functionality for nix systems (linux, macos, and the BSDs).

Basically I added a function that identifies the home directory for an arbitrary user and then threaded it into the library. The canonical way to do that in *nix systems is, I believe, to use the getpwnam_r function from libc (check out man getpwnam_r for more details).

Unfortunately this only works for the tilde and full functions (not the other versions with context) as I believe your HD: FnOnce() -> Option<P> API would need to change to something like HD: FnOnce(Option<&str>) -> Option<P> where the Option<&str> represents an arbitrary usename (or None in the case of the current user) in order to make that work and I figured you wouldn't appreciate a PR with breaking changes.

In any case, let me know what you think assuming this is something you'd like to consider.

[naufraghi]

I was opening a similar PR and I found your that does the same thing in a more complete manner, for reference my branch is add-tilde-user-expansion.

To avoid breaking the old interface, I added a new tilde_with_context_and_users function, and a users feature flag, this is the main difference I can highlight.

[bcmyers]

@naufraghi That's cool. I'm fine with whatever the author of this repository wants to do. He doesn't seem too focused on this crate (which is fine). In any case, I'll leave this PR open, but am happy for someone to close it for something better.

[netvl]

@bcmyers I'm very-very sorry for the delayed review. I've finally got some time to check this out, and I'm really impressed - the implementation quality is superb! I'm actually okay with making a breaking change - I'll release it as a new major version, which will technically be necessary anyway given the 2015->2018 edition bump.

If you think you are still willing to do this change (i.e. adding the username parameter to the callback functions), it would be great! If not, that's totally fine too - I'll try to fix this in the next few days then.

@naufraghi I've taken a look at your approach, and while it's also great, I'd say that this PR is a bit more comprehensive, especially in terms of handling of various platforms. I wonder if it would make sense to extract is as a separate crate... Does the proposed API here suit your needs too?

[naufraghi]

@netvl I'm OK with the more complete @bcmyers approach, the feature will unlock the ~user feature in nushell/nushell#1122.

Concerning a new crate, I think it may be too minimal to be used alone. I think shellexpand or dirs are the two candidate crates I can think of that can offer this feature. But given the maintenance burden that will follow on the host crate maintainer (@netvl 😅), I second the minimal crate option too.

[bcmyers]

@netvl Thanks for the comments. And no worries at all for the delay. Open source is open source and I'm just glad you spent the time to write some code and make it free for everyone! I'll have some time to look at this again in the next day or so and answer your questions.

[bcmyers]

@netvl @naufraghi. Now that I've had a moment to digest your comments, I'd be happy to work on an update that isn't constrained by small API changes. Let me give that a go and we'll see where we end up. Hopefully I'll have something to you sometime today or tomorrow. On the question of a separate crate - I'm not sure at the moment, let me play around with it.

[netvl]

@bcmyers this is awesome, thank you very much! I've pushed some commits to the otheruser-fork branch, which build on your branch a bit. One of them is unrelated to this feature and just a generic cleanup thing, while another is related to this change and does two things:

1. removes the manual enumeration of various unixes and instead relies on the standard cfg(unix) marker;
2. adds a feature flag to enable this functionality.

My reasoning is as follows: just enumerating specific target_oses makes the library unable to provide this functionality on unixes outside of this list (e.g. Redox or something like it). While using cfg(unix) does make the feature wrongly exposed on unsupporting platforms like Android or iOS, using the feature flag allows to disable it when necessary.

WDYT?

I haven't start working on the main API change, so if you do want to do it, it would be really great!

And yeah, that thing about a separate crate was just a side note, I'm totally happy with having this code living in this crate :)

[bcmyers]

All of the following goes with the important caveat that this is your library and ultimately you have the final say (since we don't know each other I thought it'd be good to say this upfront) ...

On the issue of cfg(unix) vs. cfg(any(target_os = "dragonfly",..., you hit on exactly the reason I chose the more verbose version initially. I don't think this code will work on android or iOS. I also don't think it'll work on redox (they have their own, incomplete, implementation of libc called relibc).

We could asks the users of those platforms to just turn off the home_dir functionality via a feature flag, but that would require them to do something extra that we can prevent them from needing to do at all by just not including the platforms we know upfront won't work (i.e. android, iOS, redox, and perhaps some other unixs that I'm not thinking of at the moment). As useful as feature flags can be in rust, they're also kind of a pain for the user, since 1) You have to know they exist in the first place, which is not always obvious when you want to just use a library quickly and be done with it, and 2) What the feature flags are is not easily accessible (as far as I know) on crates.io; you have to go to the github page and check out the Cargo.toml, which is not very nice for beginners and even for advanced rust users a tiny bit annoying. Don't get me wrong, feature flags can be great, I just try to avoid using them unless they're really needed.

All this to say, I would probably just stick with cfg(any(target_os = "dragonfly",..., but happy to implement it the other way. Just let me know.

[bcmyers]

I think I might be able to get Windows working as well.

[netvl]

@bcmyers thanks for the explanation! I see your point, but my main concern is that with your original approach, it won't be possible to enable the homedir code on any non-listed unixes at all, even if they do support it. Compilation targets are, in general, user-definable, and even in the rustc code base there are more unix targets than those that you've listed (e.g. this one). I don't really know if any of them support getpwnam, but the point is - they could, and user-defined targets also potentially could, and I'd prefer not to limit the user if it is possible.

I guess we can make the home-dir configuration into something like force-home-dir, and switch the condition on the nix module to something like #[cfg(any(feature = "force-home-dir", target_os = "linux", ...))]. This will enable it on the most common list of target oses which do support getpwnam, but also allow enabling it on other targets if the user wants to. WDYT?

[netvl]

As I said, breaking change is okay here, so let's change HD to FnOnce(Option<&str>) -> Option<P>, and propagate this API upwards to avoid code duplication.

[bcmyers]

Yes - That's what I'll do. Just haven't pushed a commit for it yet as I've been working on getting Windows working. The only commits so far are just merging in the changes you made to the code after I posted the initial PR; so everything is in an intermediate / not-ready-for-review state.

I'm going to push several commits today (including Windows, which works, I just need to clean it up); so let's wait on reviewing the code until all that is complete. I'll leave a message on the main thread when I think it's ready for review. Thanks!

[bcmyers]

So I just pushed a few commits. The home_dir module is probably ready for review, but I have yet to propagate API changes to lib.rs; so please don't look at lib.rs yet (just the home_dir module). I'll work on lib.rs tomorrow in order to make changes to the API and update the documentation.

Regarding the home_dir module,

• I added windows support (this is a big change)
• I added explicit redox support
• I perhaps solved our cfg issue? (TBD). The nix module is now cfg(all(unix, not(target_os = "redox"), feature = "libc")). It turns out libc does compile for android and ios with all the functions we're using, but having "libc" as a feature means that if there are some platforms out there that are "unix" but don't have the correct libc functions, users can still get the crate to compile by turning off that default feature.
• I removed the dirs dependency entirely because at this point it's not really doing much for us. The thinking is that it's always nice to remove unneeded code (although we can easily add it back in if you don't like this change)
• I added a python script (scripts/check.py) that tries to make sure we're able to compile on all the various targets I can get to cross-compile on my linux machine. If you run it, it should run cargo check on a whole bunch of different target triples. The only target triples that requires a specific OS are the ios ones, which need to run on a mac (the script will skip them if you're not on a mac).

At this point, we may want to make a decision on whether home_dir should be a separate crate. It's becoming a lot of code to maintain (especially with the addition of windows; the windows C api is awful). I'm happy to make it one if you don't want to deal with the maintenance burden of all this OS-specific code (that's essentially a bunch of C code written in Rust). If, however, you'd like to keep it in here, that's fine with me too. I'm 100% flexible.

Later on, we should probably see if we can get this functionality into the dirs crate, since that's really where it belongs, but I have no idea how difficult that would be (dirs is a pretty mature crate and so they'll care a lot about backwards compatibility). In the meantime, I think it's great if we can get this code to the point where it lives either in shellexpand or as a new, separate crate.

Happy to continue working on this. Looking foward to your thoughts on the home_dir module. In the meantime, I'll work tomorrow on lib.rs.

[netvl]

This is so awesome I don't even know where to start! Thank you very much!

It seems that having the home directory detection in a standalone crate indeed does make considerably more sense than before. With the way things are currently going, it seems that the primary functionality of the crate will take considerably less lines of code than what is needed to support just one of its relatively minor features :)

Unfortunately, for a lot of reasons I'm pretty sure I'm unfit to maintain one more crate :( As you can see, I barely manage with this one, and there is a couple of others which are in the same dire state. So if you can do that (extracting the code to a separate crate and publishing it), it would be great! If you don't want to for any reason, I can try doing that after we go through merging this PR, but I'll probably have to mark that crate as "looking for maintainers" anyway.

[netvl]

Maybe call these user_buf and path_buf, or whatever else makes more sense? It seems that they are used for a single purpose here, so it's fine to have non-generic names.

[bcmyers]

It does seem like a separate crate might be the best way to go. I probably won't be able to create one until this weekend, but I'll spin one up and then we can pull it into shellexpand as a dependency and update the API.

[netvl]

Awesome, sounds good!

[sophiajt]

We're looking forward to incorporating these updates into Nushell also. We'll try to keep out for any changes to the API we need to do to work with the new features.

[bcmyers]

Thanks for the patience everyone. I've been super busy the past two weeks, but hope to tackle this this weekend.

[bcmyers]

Wow. This became somewhat topical in the Rust community all of a sudden (referring to all the dirs news). Someone please yell at me if I don't get to this by the end of the weekend.

[sophiajt]

@bcmyers - any luck?

[mzagrabe]

What is the status of expanding ~otheruser, either in this crate (preferred!) or another crate?

[ijackson]

Hi. As discussed in #17 I am taking over maintenance of this crate. The new repository is here: https://gitlab.com/ijackson/rust-shellexpand

I looked through the discussion here and took a look at the branch too. I'm supportive of this feature but I don't think, from what I can see here, that the branch is ready to merge? I don't have the spare time to actually work on this area of the code myself, but I would be happy to review an implementation of this feature (ideally based on some other crate for getting the home directory for a user).

So, I have not merged this. And, AIUI this github repo is going to be archived soon.

I would welcome a submission as a new MR in the gitlab repo