Skip to content

Conversation

@ve-nt
Copy link

@ve-nt ve-nt commented Feb 8, 2025

I've implemented the ZON configuration file system as discussed in #146.

A couple of important points before this can be merged:
1) This does not build with Zig 0.13.0, as it relies on std.zon for parsing and serializing the ZON file. If we want to keep zigup pinned to a release instead of master we should wait until 0.14.0 is out, which hopefully wont be too long.
2) A default configuration file path for Windows has not been decided and is currently TODO. I'm not sure what is best here as I don't use Windows much. Once a decision has been made then I'm happy to implement it.
3) Not tested on Windows yet. Once point 2 is done if someone can assist with this that would be perfect, if not I can try it myself.

@ve-nt
Copy link
Author

ve-nt commented Mar 14, 2025

Okay! It seems that 0.14.0 has been released, and so we should now start having another look at this PR. The last things to do on this are:

  • Decide where to store the ZON config on Windows. An idea I had was %APPDATA%/zigup/zigup.zon. Thoughts?
  • Test on Windows

I've rebased this back onto the tip of master, and so can start getting this to a mergeable state any time.

@ve-nt ve-nt changed the title WIP: ZON Configuration ZON Configuration Mar 14, 2025
@marler8997
Copy link
Owner

%APPDATA%/zigup/zigup.zon would be fine for windows. There's also std.fs.getAppDataDir if you want to use that instead.

@ve-nt
Copy link
Author

ve-nt commented Mar 17, 2025

There's also std.fs.getAppDataDir if you want to use that instead.

This seems like the way to do it. I've added a commit to use this on Windows platforms. I've just done a rudimentary test on Windows. The config file ends up being at path C:\Users\<user>\AppData\Local\zigup\zigup.zon for me, which I think makes sense. zigup is finding the ZON file I placed there and adjusting the install_dir and path_link works as expected.

I've also bumped up the commits here and rebased them onto the tip of master, which included shuffling the XDG_HOME stuff into the Config struct with the rest of the default-path related functions.

All that being said, this should be ready for review now.

@ve-nt ve-nt force-pushed the zon-config branch 2 times, most recently from f407adb to 90a5af9 Compare March 17, 2025 18:02
@ve-nt
Copy link
Author

ve-nt commented Mar 17, 2025

Okay, realized there was some overlap between the getHomeDir functions and the XDG home dir stuff. I've taken the liberty to clean up the duplicate code and move the XDG_DATA_HOME code into Config.allocDefaultInstallDir.

@ve-nt ve-nt force-pushed the zon-config branch 4 times, most recently from 4c310d4 to 1b2a13e Compare March 17, 2025 18:36
@ve-nt
Copy link
Author

ve-nt commented Mar 17, 2025

With the removal of show-config, I have added an example path_link field setting to the example config in the README, since this will serve as the template for users going forward.

@ve-nt ve-nt force-pushed the zon-config branch 3 times, most recently from 3a085c3 to e669728 Compare March 18, 2025 14:46
ve-nt added 3 commits March 18, 2025 14:55
This struct contains the configuration for Zigup. A global object
`config` is initialized from command-line arguments and from reading in
a ZON file. Its ensured that command-line arguments always overwrite ZON
file values, and if neither are set then the default value is used.

A global (`zigup.config`) is used for storing the final configuration
because its pretty universal and gets used all over the place. Its
expected that this value gets initialized once, before we start doing
any serious work, and is never mutated again.

Some shuffling around of the parsing was needed to be done, notably the
check for the `run` command was moved after configuration is
initialized.

The `getInstallDir` and `makeZigPathLinkString` have been moved and
renamed as functions within Config, and functions using those instead
read from `zigup.config` directly. This has an added benefit of removing
some allocations.
Tilde paths (e.g. '~/.local/bin') were not expanding properly.
@ve-nt ve-nt force-pushed the zon-config branch 2 times, most recently from b2df1ea to 0e571ab Compare March 18, 2025 15:08
@ve-nt
Copy link
Author

ve-nt commented Mar 19, 2025

Did a couple tweaks to the PR here but it should be good for review now

@marler8997
Copy link
Owner

marler8997 commented Apr 20, 2025

Apologies for taking a while to respond. I was thinking about this and going through the readme and had an idea that might make this a bit simpler? I think there's probably only 1, maybe 2 options that zigup users may want to customize via a config file, the "install directory" being the main one. What if instead of a ZON config file that supports setting multiple options, we just have a single optional file for this one option? Also, since there is already an --install-dir option, the user already has a way to specify the installation directory, we're just missing way to tell zigup to retain that option for all future invocations of zigup. So what if we just add a new command-line option to do that? Something like --retain-install-dir? Then user's don't even need to know where this setting is stored, they can just read/write it via the command-line and the actual storage mechanism becomes an implementation detail. We'd also probably want to print this value in zigup's help output moving forward.

This seems like a simpler overall solution to me. We could always enhance this configuration to ZON later though if we add more options? What do you think?

@marler8997
Copy link
Owner

marler8997 commented Apr 20, 2025

FYI, I went ahead and implemented this as a command-line controlled setting for now in this commit: 3de4c86, i.e.

zigup get-install-dir
zigup set-install-dir PATH

It feels a bit simpler for the user to have to learn/manage I think.

@ve-nt
Copy link
Author

ve-nt commented Apr 21, 2025

That is a whole lot simpler for the end user, I agree. I think the only thing missing is a similar way to set the path link

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