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

Use the known-folders package to find install dir #139

Closed
wants to merge 2 commits into from

Conversation

dweiller
Copy link
Contributor

@dweiller dweiller commented Jul 15, 2024

This change makes zigup use known-folders for determining the install directory by default. A new build option default-dir has been added to control whether the default behaviour is to use the known-options package or the old behaviour. When default-dir is provided everything is the same as before, except that instead of the use of "zig" as the name of the install directory, the value of the default-dir build option is used (i.e. for the old behaviour use zig build -Ddefault-dir=zig).

When the default-dir build option is not provided the changes to behaviour are that when the --install-dir option is not provided:

  • a platform-specific standard directory is chosen, this will be the parent of the install directory for managing compilers
    • linux uses XDG_DATA_HOME, which defaults to ~/.local/share if the $XDG_DATA_HOME env var is not set
    • mac uses ~/Library/Application Support
    • windows uses the %APPDATA% env var
  • if the above platform-specific directory does not exist, zigup will report an error, rather than creating the directory
  • the zigup install (sub)directory underneath the above platform-specific directory is zigup, e.g. the default location on linux becomes ~/.local/share/zigup.

Note that I am not very familiar with Windows and macOS conventions here, but I assume known-folders is making a sensible choice; it is possible to have known-folders use the XDG spec on macOS rather than using ~/Library/Application Support.

In addition, the first commit in the PR updates use of some deprecated aliases in std that were made compile errors in the 0.13 release cycle. They were deprecated before Zig 0.12, so 0.12 still works, but with these Zig master can also build zigup. I can split this commit into a separate PR if that is preferable.

Compared to #66 (I had not noticed the apparent inactivity in that PR when I commented) I believe using known-folders is the right approach, and provides a build option as was suggested in comments on that PR to allow using original default behaviour.

The declarations `std.zig.CrossTarget`, `std.fs.MAX_PATH_BYTES`, and
`std.mem.tokenize` were made into compile errors in zig version
0.13.0-dev.33+76fb2b685. Individually, these were deprecated (but not
made compile errors) in:

  - std.mem.tokenize: 0.10.0-dev.3139+9da3a9733
  - std.zig.CrossTarget: 0.11.0-dev.1886+3179f58c4
  - std.fs.MAX_PATH_BYTES: 0.11.0-dev.3382+cd62005f1

so this change makes zigup unbuildable with versions of zig prior to
0.11.0-dev.3382+cd62005f1 (if it was previously was), but allows
building zigup with with zig versions after 0.13.0-dev.33+76fb2b685.
@dweiller dweiller marked this pull request as draft July 15, 2024 11:19
A new build option `default-dir` is provided to achieve the original
behaviour prior to this commit by building with

    zig build -Ddefault-dir=zig
@dweiller dweiller marked this pull request as ready for review July 18, 2024 03:59
@marler8997
Copy link
Owner

marler8997 commented Jul 19, 2024

I can split this commit into a separate PR if that is preferable.

Here's how I'd like to see thing split up:

  1. A PR to update deprecated aliases (so long as it passes tests would merge this immediately)
  2. A PR to add an option to change the install path (would also merge this with no fuss)
  3. A PR to change the default install directory (I'm not convinced yet that we should make this change)

One thing I note is that you've provided good documentation on what directory zigup will choose in README.md, but then you use the known-folders package which abstracts these details away. Therefore, if known-folders ever changes then the zigup README.md will no longer be valid, and furthermore, I have to go and check the logic inside of known-folders to know if it's valid in its current state. If zigup has to know these details anyway (for it's README) then why use the abstraction? I'd probably go with something more like this:

// NOTE: update README.md if any of these ever change
const default_install_path = if (build_options.install_dir) |d| d else switch (builtin.os.tag) {
    .windows => "%localappdata%\\zigup",
    .linux => "$XDG_DATA_HOME/zigup",
    .macos => "$HOME/Library/Application Support/zigup",
    else => @compileError("OS " ++ @tagName(builtin.os.tag) ++ " does not have a default install path"),
}

A few notes about windows, I think %LOCALAPPDATA% would be a more appropriate location for windows than %APPDATA%. Also I'm not convinced we should be changing the current solution on windows. If you want your compilers to go into an app data directory, you can already put your zigup.exe in the appdata folder. Or you can also create a zigup.bat script and put that anywhere in your PATH and add the --install-dir option, although, I realize some people may find this overly cumbersome.

As for all other systems, I find typing ~/zig/VERSION/... convenient in accessing the zig std library code/headers/etc and I don't think I've heard good reasons to change it. Feel free to enlighten me to convince me otherwise.

One more note, I think I'd be more amenable to adding a more convenient platform-agnostic mechanism to configuring the install path. Maybe a zigup set-install-dir INSTALL_DIR command that writes a config file somewhere? Or, maybe it updates something in the binary executable? (a resource on Windows, are there other mechanisms like this on linux/macos?)

@dweiller
Copy link
Contributor Author

dweiller commented Jul 19, 2024

  1. A PR to update deprecated aliases (so long as it passes tests would merge this immediately)
  2. A PR to add an option to change the install path (would also merge this with no fuss)

Will do.

  1. A PR to change the default install directory (I'm not convinced yet that we should make this change)

Whether the default (i.e. when just running zig build) gets changed or not, how about at least a build option to support the XDG spec on Linux and macOS? Then the question could just be whether the build option is -Duse-xdg or Dno-xdg?

One thing I note is that you've provided good documentation on what directory zigup will choose in README.md, but then you use the known-folders package which abstracts these details away. Therefore, if known-folders ever changes then the zigup README.md will no longer be valid, and furthermore, I have to go and check the logic inside of known-folders to know if it's valid in its current state. If zigup has to know these details anyway (for it's README) then why use the abstraction? I'd probably go with something more like this:

Fair point. I think there is basically 0% chance known-folders would change the location for linux, but I can't be so sure about the other operating systems. There will be more logic required then that simple switch to support XDG, but it can likely be easier to understand if we just implement the bits we need.

As for all other systems, I find typing ~/zig/VERSION/... convenient in accessing the zig std library code/headers/etc and I don't think I've heard good reasons to change it. Feel free to enlighten me to convince me otherwise.

Most people don't expect a cli tool to create non-hidden folders in their home directory by default. Personally I'd go further and say I don't really think there is much of a reason to even make hidden ones these days over using the XDG spec, but I know not everyone agrees with me there.

If cli tools make the assumption that a non-standardised and not hidden directory under $HOME is the appropriate place to store data by default, not only would this lead to an unmanageable home directory, but a user could very well already be using the chosen directory for something. It doesn't make sense for zigup to tell a user that they shouldn't/can't put their local clone of the Zig repo at ~/zig because that's where zigup is going to store its data.

One more note, I think I'd be more amenable to adding a more convenient platform-agnostic mechanism to configuring the install path. Maybe a zigup set-install-dir INSTALL_DIR command that writes a config file somewhere? Or, maybe it updates something in the binary executable? (a resource on Windows, are there other mechanisms like this on linux/macos?)

I agree a config file is the most user-friendly way to go compared with build options (I'm not familiar with Windows resources), though I wouldn't personally care for my usage as long as there are build options. I'm not aware of a mechanism to update a binary like that on Linux/macOS, which would leave the question of where to put the config file. I would strongly recommend and prefer XDG_CONFIG_HOME/zigup over something like ~/.zigup on Linux and if we did use that it seems incoherent to me if XDG_DATA_HOME/zigup was not the default install directory, but once we have a config file there could easily be a use-xdg config option if you wanted to keep the default elsewhere.

A few notes about windows, I think %LOCALAPPDATA% would be a more appropriate location for windows than %APPDATA%. Also I'm not convinced we should be changing the current solution on windows. If you want your compilers to go into an app data directory, you can already put your zigup.exe in the appdata folder. Or you can also create a zigup.bat script and put that anywhere in your PATH and add the --install-dir option, although, I realize some people may find this overly cumbersome.

I'll rework this PR - or close and open another - to just add XDG support for macOS and Linux, and leave Windows for someone more familiar with the conventions and any possible use of resources if we go for runtime config options.

@dweiller dweiller closed this Jul 20, 2024
@dweiller dweiller deleted the known-folders branch July 21, 2024 04:11
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.

2 participants