Make cargo install only mostly instead of entirely broken

[faho]

Description

(title is a bit of an overstatement, I don't want anyone to post this to HN with "fish can now be installed with cargo install!!!111eleven!!!")

Currently, if someone downloads the fish source and goes "It's a rust project, I know this!", they'll run cargo install --path . and end up with a fish install that is almost entirely broken in non-obvious ways.

As far as I know, it is impossible to disable cargo install officially. We could break it on purpose by setting an environment variable in cmake and check it in build.rs, but this would also break cargo build.

So this goes a different path:

1. If we can't read share/config.fish (that would be /usr/share/fish/config.fish in a typical package-managed install, or /usr/local/share/fish/config.fish with classical sudo make install), it prints an error and refuses to read the rest of the configuration - which would most likely throw more errors. At that point, fish is running in basically emergency mode.
2. It tells you where the data path is and to copy share/ there. If we did decide to support this, this could be a link to a "fish-4.0-share.tar.xz" that you would extract instead.
3. It defaults $PREFIX to ~/.local

The first change is split out because I believe it to be good on its own, regardless of the rest. Without it, with my own configuration, running a not-fully-installed fish would complain about fish_add_path not existing.

Together, this means the experience would be sort of like the following:

1. Run cargo install --path .
2. Fish ended up in ~/.cargo/bin/fish, run that
3. Fish tells us:

error: Fish cannot find its asset files in '/home/alfa/.local/share/fish'.
Please copy the share/ directory from the fish repository there and restart fish.
Refusing to read configuration because of this.

4. cp -r share/* ~/.local/share/fish
5. Restart fish
6. It works, with some caveats! help will just open the online documentation, which is fine. fish_config almost works if we can finagle $__fish_bin_dir (or use status fish-path, but I know that's broken on OpenBSD). man pages won't be there, so builtin --help will tell you to run help builtin.

Of course this isn't exactly ideal. It involves annoying manual steps, and especially upgrading would be a pain - you would have to install the assets again, and it's not obvious that you would have to! (we could theoretically compare mtime of config.fish vs the fish binary)

But it's simple enough to add, reasonably obvious to use, and allows people to install fish locally for themselves, which is something we historically struggled with (see our attempts at an appimage).

If cargo ever learned to install assets, we could extend this to build on that.

[Xiretza]

Wording nit:

Suggested change

	Please copy the share/ directory from the fish repository there and restart fish.\n\
	Please copy the contents of the share/ directory from the fish repository there and restart fish.\n\

[faho]

Oh, we're gonna be arguing about the wording a bunch. This is very much preliminary, a rough draft.

Personally, I would make it:

Please download https://fishshell.com/data/fish-$version.tar.xz and extract it to $datadir

But that requires us to

1. decide that this is a way of installation worth supporting
2. commit to uploading tarballs that contain just share/ to fishshell.com

There are a bunch of other ways to go about this, including embedding all our scripts with something like rust-embed, potentially in a second binary (fish_installer), and then tell you to extract them.

I would like to come to an agreement on that bigger picture first.

[faho]

Here's the complete code for a "fish_installer" binary:

diff --git a/Cargo.toml b/Cargo.toml
index 34909acb6..b757978e5 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -36,6 +36,7 @@ once_cell = "1.17.0"
 rand = { version = "0.8.5", features = ["small_rng"] }
 widestring = "1.0.2"
 git-version = "0.3"
+rust-embed = "8.2.0"
 
 [dev-dependencies]
 rand_pcg = "0.3.1"
@@ -61,6 +62,10 @@ path = "src/bin/fish_indent.rs"
 name = "fish_key_reader"
 path = "src/bin/fish_key_reader.rs"
 
+[[bin]]
+name = "fish_installer"
+path = "src/bin/fish_installer.rs"
+
 [features]
 default = []
 benchmark = []
diff --git a/src/bin/fish_installer.rs b/src/bin/fish_installer.rs
new file mode 100644
index 000000000..5bfcefc62
--- /dev/null
+++ b/src/bin/fish_installer.rs
@@ -0,0 +1,25 @@
+use rust_embed::RustEmbed;
+use std::path::PathBuf;
+use std::fs::File;
+use std::fs;
+use std::io::Write;
+
+#[derive(RustEmbed)]
+#[folder = "share/"]
+struct Asset;
+
+fn main() {
+    let dir = PathBuf::from(env!("DATADIR")).join("fish");
+    
+    for file in Asset::iter() {
+        let path = dir.join(file.as_ref());
+        println!("File: {}", path.display());
+        let _ = fs::create_dir_all(path.parent().unwrap());
+        let res = File::create(path);
+        let Ok(mut f) = res else {
+            continue;
+        };
+        let d = Asset::get(&file).unwrap();
+        f.write_all(&d.data).expect("FAILED TO WRITE");
+    }
+}

If you now run fish_installer it will self-extract all of share/ into the data directory, creating the needed directory structure.

It is about 6MB in size and takes ~60ms (with the output redirected to /dev/null).

For obvious reasons this isn't "done", it would have to gain a whole lot more error checking.

Edit: This is now at https://github.com/faho/fish-shell/tree/fish-installer, based on this branch.

Edit 2: Adding this generation to the fish binary directly, so you can do fish --generate, bloats it to 11M unstripped, 8.4M stripped from 6.1M/3.8M. That seems too much to bear.

[zanchey]

I don't think this is worth doing. It creates issues where the running fish binary, the installation in ~/.local/share and potentially an installation in /usr/local can all be different versions. These will be unpleasant and unpredictable for users and a support nightmare to untangle. It's reasonable to expect users to read the README.

[faho]

the installation in ~/.local/share and potentially an installation in /usr/local can all be different versions.

The installation in /usr/local/bin/fish would read /usr/local/share, the installation in ~/.cargo/bin/fish would read ~/.local/share. These locations would be baked into the binary, there's not really a point where they would cross streams.

Of course you can have both, but you can already have a fish in /usr/bin/fish (which reads /usr/share/fish) and one in /usr/local/bin/fish. If we made an appimage, that would also be a different version.

When you mix installation methods you have multiple installations, but I don't see how this makes the potential for confusion worse. In particular, whatever fish you run, it reads the scripts that belong to that version, so it's a consistent installation.

Note that I'm not set on ~/.local/share/fish as the datadir, it doesn't have to be that exact directory (it's a bit awkward with uninstallation because we also put history etc there). The point is more to enable cargo install to be useful, with some unfortunate manual intervention. This includes an installation in your home directory, which I know was also a pain point for people who may not have root on systems they want to use fish on.

---

Regardless of whether we make ~/.local/share the default cargo datadir and tell people to install the data files, I would like to make it print an error when share/config.fish couldn't be found, and stop reading the config. That's the first commit in this PR.

What's your opinion on that?

[zanchey]

would like to make it print an error when share/config.fish couldn't be found, and stop reading the config. That's the first commit in this PR.

What's your opinion on that?

Sensible!

[faho]

Alright, I have now added the first commit of this to master: 662fde7

That means when fish can't find share/config.fish on startup, it prints an error and refuses to read the rest of the configuration. It will still run, but tell you there's something wrong with your install.

---

I do still believe that this area is worth exploring. Something akin to my fish-installer branch is pretty close to being able to cargo install fish-shell, and that seems like a convenient installation method.

Alternatively we can look into appimages again, which should be a lot easier with #10269 - ncurses was a bit of a pain in that, IIRC.

[krobelus]

I haven't had time to look into this but writing our own installer code sounds appealing because it would open a door to getting off cmake (other things we'd need to replace are sphinx and ctest drivers, which sounds easier).
If we go this route, we can have both cmake and its replacement in the tree for a while until we are confident in making the switch.

[matklad]

As an out there idea, perhaps fish could bundle assets with the binary itself? This can't work for things like manpages, but should work for anything that fish itself needs, like https://github.com/fish-shell/fish-shell/tree/master/share/functions.

This can be feature-gated, such that cargo install installs a self-contained version, while distro package fish in a conventional way.

This could be also useful for things like statically linking a fish binary and just scping it onto whatever server you have misfortune to debug in prod :)

And even for manpages, if they are bundled, then they can be self-installed by running fish --install-manpages.

This is, of course, a terrible idea.

[faho]

As an out there idea, perhaps fish could bundle assets with the binary itself?

#10279 (comment):

Adding this generation to the fish binary directly, so you can do fish --generate, bloats it to 11M unstripped, 8.4M stripped from 6.1M/3.8M. That seems too much to bear.

That's why I would make a separate binary. Doubling the size is a lot (and it currently does not include man pages or html docs). The idea with the separate binary exists at https://github.com/faho/fish-shell/tree/fish-installer.

It's generally usable, but still requires a separate step - but if we had these files in the fish binary we would probably still require you to explicitly invoke it.

This can be feature-gated, such that cargo install installs a self-contained version, while distro package fish in a conventional way.

Here the separate binary is simply not built by cmake. cargo install will install it.

[faho]

Closing this, see #10181 (comment) for what my future ideas are.