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

Add macOS installation instructions with Homebrew #173

Merged
merged 1 commit into from
Sep 25, 2023

Conversation

Aaron-Rumpler
Copy link
Contributor

Added to Homebrew in Homebrew/homebrew-core#145662

@sharkdp sharkdp merged commit 252180c into sharkdp:master Sep 25, 2023
3 checks passed
@sharkdp
Copy link
Owner

sharkdp commented Sep 25, 2023

@Aaron-Rumpler Thank you very much for packaging Numbat! That was quick 😄

It might be worth reading what I wrote here: #170 (comment) (the first bullet point). It would be great if the Homebrew package would also do this. But I also realize that it would require a different installation method, because you don't get those files via cargo install. I'm happy for any suggestions to improve the situation for packaging.

@Aaron-Rumpler Aaron-Rumpler deleted the patch-1 branch September 25, 2023 08:34
@Aaron-Rumpler
Copy link
Contributor Author

Installing the modules directory should be fairly easy, however it won't be into /usr/share/numbat. It'll end up symlinked into $HOMEBREW_PREFIX/share/numbat, where $HOMEBREW_PREFIX is usually /usr/local on macOS x86_64 and /opt/homebrew on macOS aarch64 (though those are only the defaults, and Homebrew supports Linux as well). I'm not sure the best way to handle this in terms of Numbat being able to find that (though I'll have a look through the Homebrew documentation).

@Aaron-Rumpler
Copy link
Contributor Author

Looks like this can (and usually is) done at compile-time. So I just need to be able to pass in the path to the .../share/numbat directory during compilation.

@sharkdp
Copy link
Owner

sharkdp commented Sep 25, 2023

Ok, just to see if we're on the same page:

Numbat tries to load its modules from a (ordered) list of 'module' paths. New paths can be added at runtime by the user via $NUMBAT_MODULES_PATH. There are also "hard coded" user paths (e.g. ~/.config/numbat/modules). But currently there is just one system-wide hard coded path for macOS: /usr/share/numbat/modules. Importantly, if some (or all) of the paths in this list are not existent, this is not an error.

If I understand it correctly, there are two approaches we could take:

  • We simply add /usr/local/share/numbat/modules and /opt/homebrew/share/numbat/modules as additional hard coded system-wide module-paths for macOS.
  • We add a way (for package maintainers) to add additional modules paths to that hard-coded list inside the binary at compile time, for example by using a ADD_NUMBAT_MODULES_PATH environment variable that would be read at build time.

Or we could do both.

What do you think?

@Aaron-Rumpler
Copy link
Contributor Author

Aaron-Rumpler commented Sep 25, 2023

I definitely prefer the compile-time approach. Apart from providing more flexibility, it will also support users who have installed Homebrew into non-default locations (and means we're not hardcoding assumptions about Homebrew into Numbat), as well as any other cases of installing into other locations.

What about replacing the hardcoded /usr/share/numbat/modules and C:\Program Files\numbat\modules (and associated platform check) with whatever gets passed at compile-time (defaulting to /usr/share/numbat/modules and C:\Program Files\numbat\modules on UNIX and Windows respectively)?

One point to make though is that Homebrew will put the .../share/numbat directory at something like /opt/homebrew/Cellar/numbat/1.6.2/share/numbat and symlink that into /opt/homebrew/share. So what will get passed at compile-time will be /opt/homebrew/Cellar/numbat/1.6.2/share/numbat (or similar). /opt/homebrew/share/numbat isn't actually guaranteed to exist, as Homebrew provides the option to not symlink an installed formula (though that would be rare in practice for a command-line utility, unless there was a conflict with another formula or one wanted to install both the latest release and install from HEAD).

sharkdp added a commit that referenced this pull request Sep 25, 2023
@sharkdp
Copy link
Owner

sharkdp commented Sep 25, 2023

Added in c666cd7. I also wrote some documentation for package maintainers: https://numbat.dev/doc/cli-installation.html#guidelines-for-package-maintainers

The standard paths like /usr/share/numbat are still included, as I would like to keep this section valid.

@sharkdp
Copy link
Owner

sharkdp commented Sep 25, 2023

So what will get passed at compile-time will be /opt/homebrew/Cellar/numbat/1.6.2/share/numbat (or similar)

Oh, ok. I don't think that's a problem, except for the longer path in the error message.

@Aaron-Rumpler
Copy link
Contributor Author

Aaron-Rumpler commented Sep 25, 2023

I've got the module directory installing with Homebrew. Should we include the examples as well?

@Aaron-Rumpler
Copy link
Contributor Author

Aaron-Rumpler commented Sep 25, 2023

The standard paths like /usr/share/numbat are still included, as I would like to keep this section valid.

I'd argue that those paths are specific to only an install in the default location, and that it would be desirable for additional installs in different locations to not reference them (to keep them isolated from the install in the default location). For example, if someone has the latest stable release installed in the default location, and a development build installed elsewhere, that the development build shouldn't reference the default install's modules (especially as they may differ between different versions?). What do you think?

@sharkdp
Copy link
Owner

sharkdp commented Sep 26, 2023

I've got the module directory installing with Homebrew. Should we include the examples as well?

👍

I think we can leave out the examples for now. There are too many things in there, like "this should fail with an error" examples and prelude-tests. Those are not interesting for most users. The more polished examples are available in the documentation (https://numbat.dev/doc/examples.html).

I was thinking if it would be valuable to install the documentation, as static HTML files. But I'm not really sure how many users would actually read that. It's probably much easier to just refer to https://numbat.dev/doc/, and assume that an internet connection is available 99% of the time.

sharkdp added a commit that referenced this pull request Sep 26, 2023
@sharkdp
Copy link
Owner

sharkdp commented Sep 26, 2023

I'd argue that those paths are specific to only an install in the default location, and that it would be desirable for additional installs in different locations to not reference them (to keep them isolated from the install in the default location). For example, if someone has the latest stable release installed in the default location, and a development build installed elsewhere, that the development build shouldn't reference the default install's modules (especially as they may differ between different versions?). What do you think?

You convinced me: 87a4ed5

@sharkdp
Copy link
Owner

sharkdp commented Sep 26, 2023

This is now available in 1.6.3. Thank you

@Aaron-Rumpler
Copy link
Contributor Author

Aaron-Rumpler commented Sep 27, 2023

Great. I'll update the formula and put in a PR.

One other thought, for a case like installing just the executable (so no modules directory), setting NUMBAT_SYSTEM_MODULE_PATH= works for removing the default path, but I assume that would also add an empty path to paths? Maybe we should check for a blank string and only push it onto paths if it's not blank?

@sharkdp
Copy link
Owner

sharkdp commented Sep 27, 2023

One other thought, for a case like installing just the executable (so no modules directory), setting NUMBAT_SYSTEM_MODULE_PATH= works for removing the default path, but I assume that would also add an empty path to paths? Maybe we should check for a blank string and only push it onto paths if it's not blank?

45fa31c

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 this pull request may close these issues.

None yet

2 participants