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

Follow more XDG conventions #4542

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

booniepepper
Copy link
Contributor

What does this PR do?

I'm new to the codebase but I got hyped about the 1.0 release at the last hour

(If I'm following right) This should be the last little bit needed to close #1678 and prevent future breakage based on install locations

It also builds off of #4438 from @lgarron

  • Documentation or TypeScript types (it's okay to leave the rest blank in this case)
  • Code changes

How did you verify your code works?

  • I ran make zig and it worked
  • I or my editor ran zig fmt on the changed files
  • I included a test for the new code, or an existing test covers it
    • Not sure yet, I've only cloned the repo about an hour ago :)

lgarron and others added 3 commits September 8, 2023 00:44
… var is set).

This addresses the comment at oven-sh#1678 (comment)

> … I have yet to see see any other program use a `.` prefix for folders
> *inside* the XDG convention folders. This may cause the folder to be
> unexpectedly hidden when viewing it in a file explorer app or running
> `ls` without `-a`. (That is, someone may expect `~/.cache` to be
> hidden — but that all folders are visible once they are viewing that
> hidden folder.)

The only previous workaround would have been to set `$BUN_INSTALL`, but
that affects more than the cache directory. (And people who have been
using `$XDG_CACHE_HOME` presumably want to opt into the convention that
all cache directories are grouped into a single place that is easy to
audit and clean, separate from all config data.)

Since this:

- only affects the subset of users who have explictly
opted into `$XDG_CACHE_HOME`, and
- only affects cache data, which is meant to be safe to abandon/lose at
  any moment,

… it shouldn't be harmful to leave the old directory lying around and
allow a new one to be created. But it would also be possible to move the
existing directory with an additional change.
@lgarron
Copy link
Contributor

lgarron commented Sep 7, 2023

I think this would be a nice step, but nit on the title: this PR currently only covers the cache dir. There are also various other config and data paths in Bun that don't follow the XDG conventions.

In addition, some people believe would probably prefer for programs not to strictly adhere to the Linux-centered XDG conventions, but instead to use platform-specific defaults (e.g. ~/Library/Caches on macOS) when the XDG vars aren't set. For example, see https://crates.io/crates/dirs/1.0.5 for a fairly thorough treatment of this in Rust.

@booniepepper
Copy link
Contributor Author

Ah I didn't know there's more places. I've used that crate in some projects and it's def fantastic. I've referred to it when doing stuff outside Rust too

In that case... I guess I'd expect this to be switching on the OS in the Zig layer before getting to XDG_ and HOME handling. I don't know how many other layers (c++? js?) also need updates. I'm about to sign off for the night, but feel free to run with this or even cancel my PR. Some of my assumptions were off

@booniepepper booniepepper changed the title Finish following XDG conventions Follow more XDG conventions Sep 11, 2023
@booniepepper
Copy link
Contributor Author

@lgarron I updated the PR title; aside from that, do you think this looks like a good incremental step forward?

@ZerdoX-x
Copy link

I would really like to see XDG specs implementation first. I hope it won't spread over months in order to include support for all platforms... Also I think most of MacOS and Windows users don't really care about where their apps store data, comparing to Linux and BSD. It should be implemented, but Linux specs go first imo.

@lgarron are changes only include install script? if yes, it's probably very easy to finish in order to get merged.

I can't imagine new fresh tool like bun can end up getting to this table: https://wiki.archlinux.org/title/XDG_Base_Directory

@booniepepper
Copy link
Contributor Author

booniepepper commented Sep 11, 2023

I can't imagine new fresh tool like bun can end up getting to this table: https://wiki.archlinux.org/title/XDG_Base_Directory

I added bun to the "Support > Partial" table, along with a workaround shell export that I think is sufficient to work around until the related XDG issue is resolved. It's a wiki, anyone can edit it. :) Feel free to update it later, I'm not an Arch user so I may forget about it

@lgarron
Copy link
Contributor

lgarron commented Sep 11, 2023

I can't imagine new fresh tool like bun can end up getting to this table: https://wiki.archlinux.org/title/XDG_Base_Directory

I added bun to the "Support > Partial" table, along with a workaround shell export that I think is sufficient to work around until the related XDG issue is resolved. It's a wiki, anyone can edit it. :) Feel free to update it later, I'm not an Arch user so I may forget about it

I can't practically sign up for the wiki, as I don't seem to have access to a system that allows me to answer the anti-spam filter.

However, I would suggest something like this:

It is possible to set export BUN_INSTALL="$XDG_DATA_HOME"/bun. However, bun will prioritize using $XDG_CONFIG_HOME, $XDG_CACHE_HOME, and/or $XDG_DATA_HOME when these have been set.

@ZerdoX-x
Copy link

I added bun to the "Support > Partial" table

Thank you :)

I'm not an Arch user so I may forget about it

Me neither but I have followed the issue and I would be very happy to remove this variable from my environment. I've left a comment above variable so that's not a problem 👍

along with a workaround shell export that I think is sufficient to work around until the related XDG issue is resolved.

However, I would suggest something like this:

Yes, thank you. I already read install script and shared this finding in issue thread: #1678 (comment)

@booniepepper I also left some maybe useful info for you about XDG specs implementation: #1678 (comment)


I can't practically sign up for the wiki, as I don't seem to have access to a system that allows me to answer the anti-spam filter.

If you need it, you can just use minimal alpine docker image. This is the way you register on gentoo wiki also for example.

And current captcha for arch wiki is

BIQC4LJNFYQCAIBAEAQCAIBAEAQCAIBAEAQCAICQMFRW2YLOEB3DMLRQFYZCALJANRUWEYLMOBWS

@lgarron
Copy link
Contributor

lgarron commented Sep 11, 2023

And current captcha for arch wiki is
BIQC4LJNFYQCAIBAEAQCAIBAEAQCAIBAEAQCAICQMFRW2YLOEB3DMLRQFYZCALJANRUWEYLMOBWS

Thanks, that worked! 😀

Yes, thank you. I already read install script and shared this finding in issue thread: #1678 (comment)
BIQC4LJNFYQCAIBAEAQCAIBAEAQCAIBAEAQCAICQMFRW2YLOEB3DMLRQFYZCALJANRUWEYLMOBWS

I was trying to make the point that setting $BUN_INSTALL can have counter-intuitive effects, in that it can override other settings like the cache dir:

https://github.com/oven-sh/bun/blob/e09129074883f7a73c0fa7b3af833c3d56e3b8da/src/install/install.zig#L3656C26-L3664

So right now it's not simple as "set $BUN_INSTALL and everything will get better". In fact, I myself specifically avoid setting that var, so that my bun cache is with all the other caches. (For caches in particular, you can actually set $BUN_INSTALL_CACHE_DIR as an even higher priority override. But this gets super hairy, whereas setting explicit values for the XDG dir vars results in the correct behaviour as long you don't also set $BUN_INSTALL.)

I think we're probably all agreed in this thread that the least confusing if all of bun fully followed the XDG dir conventions (potentially with platform-sensitive defaults) out of the box, though.

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.

Follow XDG Base Directory Specification
3 participants