edubkendo / atom-racer

Intelligent code completion for Rust in the Atom Editor. Requires Racer.
MIT License
115 stars 24 forks source link

Don't default to hardcoded absolute paths #59

Open mtorromeo opened 8 years ago

mtorromeo commented 8 years ago

Since the dawn of time Unix supported searching for binary in $PATH. Please just use racer as the default value for the utility instead of /usr/local/bin/racer which is still worse than the standard /usr/bin/racer.

Also maybe also use /usr/src/rust/src? Any official package, when provided by a Linux distro, would use this path anyway.

Thanks!

alkama commented 8 years ago

Hello, Just a few notes.

Of course, that's what we planned first! In fact, initially, we wanted to rely on not having any settings and just silently use racer (from the PATH) and RUST_SRC_PATH environment variable (that the users would have setup on their system).

Sadly, it turned not that simple, at least for OSX, and would have required a lot of users fiddling. Last time I checked, the environment variables that Atom sees is not the users one, but the systems one (on OSX). I think it comes from the fact that app launching is done by launchd (but I may be wrong). For exemple, last time I checked, setting a RUST_SRC_PATH in your bashrc or profile wont make it available for Atom to use and see. To have it happen, the user would have to use a line like launchctl setenv RUST_SRC_PATH [the path] and they also would have to open a terminal after each reboot before launching Atom for that line to execute.

In the end, we turned to settings, providing explicit pathes that anyone can change. We used default values that made sense back when rust and racer were evolving quickly and were not available as distro packages and were also pretty common on OSX.

About /usr/src and /usr/bin, on OSX El Capitan, this path is even protected by "SIP" and even root cannot alter it. So racer and rust source have no chance to ever install there. And I dont even mention windows.

And if I focus on linux only, I highly doubt that there's a common sensible place that all distro use to install things either. Not even talking about when compiling things by hand.

Lastly, from experience, we had a LOT of issues where users put directories instead of the full path in the settings field for racer. Even when showing explicit requirements, users get confused. I'm a bit scared of what might happen if they dont even SEE a real path there.

So hardcoded default values feel just as sensible (providing explicit requirements) as system guessed paths that wont ever work for a lot of users.

Anyway, if you're confident it's a better option, then you're welcome to change it and provide a pull request.

mtorromeo commented 8 years ago

I completely undestand. I still think there's a better solution though.

For the racer tool you could use a 2 steps process where you first run which racer and if successful use this path or fallback to /usr/local/bin/racer. (BTW, OSX doesn't have /usr/local/bin in the PATH?)

Same for RUST_SRC_PATH. If the env variable is set use it. If not, use /usr/local/src/rust/src. Even better you could detect the OS at runtime and default to /usr/local/src/rust/src on OSX and /usr/src/rust/src as fallback.

edubkendo commented 8 years ago

We would happily consider a robust pull request.

Luthaf commented 7 years ago

This is a grip point for me because I am synchronizing my atom setting between an OS X machine and a Linux machine, and hard coded path do not play well here ...

I could give it a try, but I am not sure I understood all the constraints here.

What is the problem with having racer as the default path, and letting the users override it to ~/.cargo/bin/racer or /usr/local/bin/racer as they want? This package would work with no change for previous users (the setting are conserved I believe). For new users, this would work out of the bow for people launching atom in command line on OS X (I do this) and on Linux, and others would need to set the path they need.

I think this is better than relying on /usr/local/bin/racer which is the brew installation path, because even on OS X people could use cargo install to get racer.

bronson commented 7 years ago

last time I checked, setting a RUST_SRC_PATH in your bashrc or profile wont make it available for Atom to use and see.

Happily, this changed a year ago: http://blog.atom.io/2016/03/17/atom-1-6-and-1-7-beta.html#environment-patching-on-os-x

It's now common for Atom packages to read from the environment.

I'll try to find time for a PR. Very much hoping someone can beat me to it. :)

@Luthaf I have the same problem: my Atom config is shared between Mac and Linux. Everything works except for this package.

bronson commented 7 years ago

Looks like the PATH fix is already written: https://github.com/edubkendo/atom-racer/pull/60