Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Borked on *nix that isn't Linux nor Redox #1

Closed
ghost opened this issue May 26, 2019 · 19 comments
Closed

Borked on *nix that isn't Linux nor Redox #1

ghost opened this issue May 26, 2019 · 19 comments

Comments

@ghost
Copy link

ghost commented May 26, 2019

dirs_sys::user_dir is only defined for Linux and Redox.

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:17:56
   |
17 | pub fn audio_dir()      -> Option<PathBuf> { dirs_sys::user_dir("MUSIC") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:18:56
   |
18 | pub fn desktop_dir()    -> Option<PathBuf> { dirs_sys::user_dir("DESKTOP") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:19:56
   |
19 | pub fn document_dir()   -> Option<PathBuf> { dirs_sys::user_dir("DOCUMENTS") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:20:56
   |
20 | pub fn download_dir()   -> Option<PathBuf> { dirs_sys::user_dir("DOWNLOAD") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:22:56
   |
22 | pub fn picture_dir()    -> Option<PathBuf> { dirs_sys::user_dir("PICTURES") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:23:56
   |
23 | pub fn public_dir()     -> Option<PathBuf> { dirs_sys::user_dir("PUBLICSHARE") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:24:56
   |
24 | pub fn template_dir()   -> Option<PathBuf> { dirs_sys::user_dir("TEMPLATES") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`

error[E0425]: cannot find function `user_dir` in module `dirs_sys`
  --> /data/data/com.termux/files/home/.cargo/registry/src/github.com-1ecc6299db9ec823/dirs-2.0.0/src/lin.rs:25:56
   |
25 | pub fn video_dir()      -> Option<PathBuf> { dirs_sys::user_dir("VIDEOS") }
   |                                                        ^^^^^^^^ not found in `dirs_sys`
@ghost ghost changed the title Borked on Android Borked on *nix that isn't Linux nor Redox May 26, 2019
@soc
Copy link
Collaborator

soc commented May 26, 2019

Hey @kijikeki,

thank you for the report!

May I ask which kind of Unix are you running? Would you propose widening the platforms eligible to use XDG user dirs or do you have a different solution in mind?

Thanks,

Simon

Edit: Oh, I see this is Android – would there be more appropriate directories to use there?

@fnichol
Copy link
Contributor

fnichol commented May 27, 2019

Howdy there! I found a similar result when building cargo-make (this crate is in the dep tree) on FreeBSD 11.2/stable Rust (current CI failure: https://cirrus-ci.com/task/6627659522506752). I suspect this isn't a huge fix so I'll likely dig in later today and see if I can't help with a fix. For the meantime I figured I'd add context here.

@soc
Copy link
Collaborator

soc commented May 27, 2019

Thank you @fnichol!
I guess it might be the best idea to revert the function to the if-def from version 1, I think this change was not intentional, but I can't remember anymore.

@ghost
Copy link
Author

ghost commented May 27, 2019

@soc Hmm, I wanted to use this crate along with dirs in the Linux environment and Terminal app, Termux.
As an APK... I'm unsure honestly, there are multiple directories that can be used as "user dirs" and such. (See Context)

@soc
Copy link
Collaborator

soc commented May 28, 2019

@kijikeki Can you comment on how useful these functions are on Android?

I'd expect that even if it compiled that the XDG user dirs all returned None, and I'm not sure whether the XDG base dirs are returning anything useful either.

If you install such an APK with native code, do you even have read access/write access to $HOME (or does it fail at lookup?).
Same with the config, cache, ... dirs?

I'm currently considering either

  • making most function return None
  • not handling Android in a special way, just letting it use the Linux code path.

What do you think?

@ghost
Copy link
Author

ghost commented May 29, 2019

As an APK, None should be returned, yeah.
But as a binary, handle it the standard linux way.
If that's possible (build.rs or something).
Otherwise yeah, don't handle Android in a special way. Linux code should be fine.

@fnichol
Copy link
Contributor

fnichol commented May 29, 2019

I think the Linux code path should be fine as well. Looks like you'd essentially get XDG behavior which works most Unix derivatives such as FreeBSD (in my case), even on macOS (personally, I'm liking the ~/.config/* locations for software no matter the platform for CLI tooling)

@ghost
Copy link
Author

ghost commented May 29, 2019

Yeah, well I got a ~/.config/* dir on my phone, haha.

fnichol added a commit to fnichol/dirs-sys-rs that referenced this issue May 29, 2019
This change broadens the list of targets which will conditionally
compile the `user_dir` and `user_dirs` functions. Prior to this change,
only the Linux and Redox OS targets will compile an implementation
meaning that other Unix derivatives such as Android, FreeBSD, NetBSD,
Darwin, etc. would not have a compiled implementation of these
functions. Once paired and consumed by the upstream `dirs` crate this
would lead to a compilation failure on these "extra" targets. This
change allows all Unix family targets in addition to the Redox OS target
to compile the same implementation.

Closes dirs-dev#1

Signed-off-by: Fletcher Nichol <[email protected]>
@fnichol
Copy link
Contributor

fnichol commented May 29, 2019

Well, I have a proposed fix in #2 linked above which does open the target list for user_dir and user_dirs to essentially all Unix targets. Based on my reading of rust-lang/rust#60139, it's possible that Redox is either not in the target_family = "unix" or not yet a member so I tried to carefully select both. Worst case is that it is an overlapping cfg selector which would be evaluated to #[cfg(any(true, true))] in the end.

Would this work in all our use cases or do we need to be more surgical with Android?

Note that I tested this by creating a new crate (via cargo new test-dirs), added the dirs crate, and patched the dirs-sys crate dependency to point to my local fork. Then I built it on a FreeBSD system and watched it compile cleanly 😄 Based on the test suite in this crate, the better place to verify this fix seemed to be in dirs as it expects to consume a dirs_sys::user_dir function.

@ghost
Copy link
Author

ghost commented May 29, 2019

Actually, is dirs even used in any rust APKs?
I don't think anyone would be using the crate for an APK. They'll most likely use JNI to get the appropriate paths from the application's context. Perhaps this could be implememted, but I assume it's a fair amount of work.

@ghost
Copy link
Author

ghost commented May 29, 2019

If it is implemented, perhaps make it a feature, leave it up to the developer on what to do? E.g.:

[features]
android-apk = [ "jni" ]

@soc
Copy link
Collaborator

soc commented May 30, 2019

@fnichol Wouldn't this change also enable macOS?

@fnichol
Copy link
Contributor

fnichol commented May 30, 2019

It would, technically although only for this crate. With a stock macOS system, calls to user_dir return None (as it doesn't export XDG_CONFIG_HOME or contain $HOME/.config/user-dirs.dirs by default). As the crate README.md calls out, it's expected that users consume this crate through a higher level crate such as dirs or directories which don't call this function on macOS systems in their implementations. And finally, this leaves all Unix derivatives with a present and compilable implementation which doesn't panic and returns something reasonable.

I tested this out by creating another test crate (with a build.rs to echo out the compiled target triple) to output the results of calling user_dirs a couple of times:

const TARGET: &str = env!("TARGET");

fn main() {
    println!("target triple: {}", TARGET);
    println!("desktop_dir: {:?}", dirs_sys::user_dir("DESKTOP"));
    println!("pictures_dir: {:?}", dirs_sys::user_dir("PICTURES"));
}

This was the build.rs to show where TARGET came from:

fn main() {
    println!(
        "cargo:rustc-env=TARGET={}",
        std::env::var("TARGET").unwrap()
    );
}

And the output, first on my Mac, and then a second time in the Rust Docker container which is based on Debian/Stretch:

> cargo run
   Compiling libc v0.2.55
   Compiling cfg-if v0.1.9
   Compiling test-dirs-sys v0.1.0 (/Users/fnichol/Projects/github.com/soc/test-dirs-sys)
   Compiling dirs-sys v0.3.1 (/Users/fnichol/Projects/github.com/soc/dirs-sys-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 2.44s
     Running `target/debug/test-dirs-sys`
target triple: x86_64-apple-darwin
desktop_dir: None
pictures_dir: None

# Sorry for the directory changing steps, but I needed my local fork present when
# compiling the project inside the Docker container
> pushd .. && docker run --rm -ti -v $(pwd):/src -w /src/test-dirs-sys rust cargo run; popd
~/Projects/github.com/soc ~/Projects/github.com/soc/test-dirs-sys
    Updating crates.io index
  Downloaded cfg-if v0.1.9
  Downloaded libc v0.2.55
   Compiling libc v0.2.55
   Compiling cfg-if v0.1.9
   Compiling test-dirs-sys v0.1.0 (/src/test-dirs-sys)
   Compiling dirs-sys v0.3.1 (/src/dirs-sys-rs)
    Finished dev [unoptimized + debuginfo] target(s) in 44.46s
     Running `target/debug/test-dirs-sys`
target triple: x86_64-unknown-linux-gnu
desktop_dir: None
pictures_dir: None

While I don't personally think this is too bad, I could treat macOS and any other relevant targets specifically to return None for the user_dir and user_dirs implementations. In that case, I'd likely double down on using the cfg_if! macro to deal with the exceptions first, then follow with an else if for the remaining Unix targets.

I'm more than happy to go any which way here. Thoughts?

Thanks!

@soc
Copy link
Collaborator

soc commented May 30, 2019

@fnichol Funny timing, I just committed changes, but didn't hit the publish button yet: 51b0e46

What do you think?

I'd prefer not allowing macOS in the ifdef, because the implementation resides in a file called xdg_user_dirs, and just adding more stuff (e. g. in a different place) for macOS doesn't make sense imho, because dirs-rs already provides the functionality itself.

@fnichol
Copy link
Contributor

fnichol commented May 30, 2019

@soc Awesome, I think that will work nicely! Thanks so much for working through this, my CI should happily light back up green again. 🍰

@soc
Copy link
Collaborator

soc commented May 30, 2019

Ok, great! I published dirs-sys-rs 0.3.2, and after the CI is green for dirs-rs and directories-rs I'll publish them too.

@soc
Copy link
Collaborator

soc commented May 30, 2019

Things are green, publishing. Thanks!

@ghost
Copy link
Author

ghost commented May 31, 2019

Green here on Android too, thanks a lot!

@soc
Copy link
Collaborator

soc commented Jun 3, 2019

Great, thank you!

Closed in 51b0e46.

@soc soc closed this as completed Jun 3, 2019
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 a pull request may close this issue.

2 participants