Better XDG defaults for Windows #5943

nojb · 2022-07-01T09:18:21Z

This sets the following XDG defaults on Windows:

XDG_CONFIG_HOME --> %LOCALAPPDATA%
XDG_DATA_HOME --> %LOCALAPPDATA%
XDG_CACHE_HOME --> FOLDERID_InternetCache

A C call is needed to get the right path for FOLDERID_InternetCache. This is included in the PR, but perhaps it is considered to be too much trouble? Opinions welcome.

cc @dra27 @jonahbeckford

Fixes #5808

otherlibs/xdg/xdg_stubs.c

otherlibs/xdg/xdg.ml

rgrinberg · 2022-07-01T15:29:28Z

Could you add a CHANGES entry?

jonahbeckford · 2022-07-02T00:05:42Z

Thanks. LGTM, notwithstanding rgrinberg's comments.

Tested the PR (really for my own notes because I haven't done a Dune PR):

Z:\source\dune>opam dkml init
Z:\source\dune>(& opam env --switch=$PWD) -split '\r?\n' | ForEach-Object { Invoke-Expression $_ }
Z:\source\dune>opam install bos utop
Z:\source\dune>gh pr checkout 5943
Z:\source\dune>with-dkml ocaml bootstrap.ml -j 8
Z:\source\dune>with-dkml .\dune.exe build @otherlibs/xdg/all

Z:\source\dune>utop
utop # #require "bos";;
utop # #directory "_build/default/otherlibs/xdg";;
utop # #directory "_build/default/otherlibs/xdg/.xdg.objs/byte";;
utop # #load_rec "xdg.cma";;
utop # let xdg = Xdg.create ~win32:true ~env:Bos.OS.Env.var () ;;
val xdg : Xdg.t = <abstr>
utop # Xdg.cache_dir xdg;;
- : string = "C:\\Users\\beckf\\AppData\\Local\\Microsoft\\Windows\\INetCache"
utop # Xdg.config_dir xdg;;
- : string = "C:\\Users\\beckf\\AppData\\Local"
utop # Xdg.data_dir xdg;;
- : string = "C:\\Users\\beckf\\AppData\\Local"
utop # Xdg.runtime_dir xdg;;
- : string option = None

emillon · 2022-07-04T12:14:42Z

xref to ocaml-community/utop#362 which needs something similar.

zapashcanon · 2022-07-05T15:11:24Z

I don't remember the details because I wrote directories a long time ago, starting from how it's implemented in rust and java here https://github.com/dirs-dev ; but :

XDG_CONFIG_HOME --> %LOCALAPPDATA%
XDG_DATA_HOME --> %LOCALAPPDATA%

Shouldn't it be appdata instead of localappdata (which is the one used for the cache) ? See there.

Moreover IIRC in some cases using %APPDATA% and not going through the C function would lead to an incorrect result.

Also, I don't understand why you're using &FOLDERID_InternetCache and not something like shgetknownfolderif(LocalApplicationData)/cache (see here and there).

nojb · 2022-07-05T15:29:30Z

Shouldn't it be appdata instead of localappdata (which is the one used for the cache) ? See there.

%APPDATA% is for "Roaming" data, that must travel with the user on different computers (this is a Windows-specific distinction). Data that is local to the user and the machine should go into %LOCALAPPDATA%.

Moreover IIRC in some cases using %APPDATA% and not going through the C function would lead to an incorrect result.

Thanks. I could add that logic since I'm already using a C binding, but on the other hand, it can be "incorrect" in the same way that HOME can be incorrect if the user changes it before calling the program. Not sure it is worth protecting against that.

Also, I don't understand why you're using &FOLDERID_InternetCache and not something like shgetknownfolderif(LocalApplicationData)/cache (see here and there).

See #5808 (comment)

zapashcanon · 2022-07-05T15:40:21Z

%APPDATA% is for "Roaming" data, that must travel with the user on different computers (this is a Windows-specific distinction). Data that is local to the user and the machine should go into %LOCALAPPDATA%.

Yes, that's why I believe the config dir and the data dir should be in appdata and not localappdata, which is not the case in the PR, right ?

nojb · 2022-07-05T16:24:24Z

%APPDATA% is for "Roaming" data, that must travel with the user on different computers (this is a Windows-specific distinction). Data that is local to the user and the machine should go into %LOCALAPPDATA%.

Yes, that's why I believe the config dir and the data dir should be in appdata and not localappdata, which is not the case in the PR, right ?

Sure, this may be a possibility. @jonahbeckford do you have an opinion as to whether %APPDATA% or %LOCALAPPDATA% should be used?

dra27 · 2022-07-05T16:25:18Z

If one is trying to cater for roaming vs local settings properly, config should be loaded from both places and merged. display, for example is a setting you'd expect to be able to roam, cache is one you probably don't want to. However, I think local is a sensible default, especially as it's possible to override it.

What does Dune write under XDG_DATA_HOME? I agree it's likely that that should be local - on behalf of myself and anyone else with a roaming profile, it's irritating when thousands of files get blatted into my roaming profile for no particularly good reason!

FWIW, especially as you have the function already, I'd use SHGetKnownFolderPath for all of them - it's an anti-feature to be able to override the appdata directory with an environment variable (the environment variable exists to allow batch files to read the location, not for properly-written programs to use).

nojb · 2022-07-05T16:31:32Z

Thanks David for the input. I'll amend to use the C binding every time. But I think I am keeping %LOCALAPPDATA% for the reasons you mention. At worst it is the same situation as on Unix :)

nojb · 2022-07-11T22:04:50Z

Could you add a CHANGES entry?

Done.

nojb · 2022-07-11T22:06:00Z

I believe all points have been addressed. A formal approval would be appreciated, otherwise I'm planning to merge in the next few days.

nojb · 2022-07-11T22:11:29Z

The Windows CI is failing when building the bootstrap binary:

I cannot reproduce locally so far. Any ideas @dra27?

MisterDA · 2022-07-15T10:41:55Z

This comment, possibly related, suggests adding -luuid.

emillon · 2022-07-19T13:00:03Z

@nojb I rebased and added -luuid and this bootstraps now. I suppose we don't really need xdg support during bootstrap, do you know how to replace this with a dummy version for bootstrap?

emillon · 2022-07-19T13:00:43Z

New error after bootstrap is:

** Cannot resolve symbols for otherlibs/xdg\libxdg_stubs.a(xdg_stubs.o):
 FOLDERID_InternetCache
 FOLDERID_LocalAppData

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Signed-off-by: Etienne Millon <me@emillon.org>

@esope

…ne-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.4.0) CHANGES: - Make `dune describe` correctly handle overlapping implementations for virtual libraries (ocaml/dune#5971, fixes ocaml/dune#5747, @esope) - Building the `@check` alias should make sure the libraries and executables don't have dependency cycles (ocaml/dune#5892, @rgrinberg) - [ctypes] Add support for the `errno` parameter using the `errno_policy` field in the ctypes settings. (ocaml/dune#5827, @droyo) - Fix `dune coq top` when it is invoked on files from a subdirectory of the directory containing the associated stanza (ocaml/dune#5784, fixes ocaml/dune#5552, @ejgallego, @rlepigre, @Alizter) - Fix hint when an invalid module name is found. (ocaml/dune#5922, fixes ocaml/dune#5273, @emillon) - The `(cat)` action now supports several files. (ocaml/dune#5928, fixes ocaml/dune#5795, @emillon) - Dune no longer uses shimmed `META` files for OCaml 5.x, solely using the ones installed by the compiler. (ocaml/dune#5916, @dra27) - Fix handling of the `(deps)` field in `(test)` stanzas when there is an `.expected` file. (ocaml/dune#5952, ocaml/dune#5951, fixes ocaml/dune#5950, @emillon) - Ignore insignificant filesystem events. This stops RPC in watch mode from flashing errors on insignificant file system events such as changes in the `.git/` directory. (ocaml/dune#5953, @rgrinberg) - Fix parsing more error messages emitted by the OCaml compiler. In particular, messages where the excerpt line number started with a blank character were skipped. (ocaml/dune#5981, @rgrinberg) - env stanza: warn if some rules are ignored because they appear after a wildcard rule. (ocaml/dune#5898, fixes ocaml/dune#5886, @emillon) - On Windows, XDG_CACHE_HOME is taken to be the `FOLDERID_InternetCache` if unset, and XDG_CONFIG_HOME and XDG_DATA_HOME are both taken to be `FOLDERID_LocalAppData` if unset. (ocaml/dune#5943, fixes ocaml/dune#5808, @nojb)

nojb · 2022-07-20T19:10:59Z

@nojb I rebased and added -luuid and this bootstraps now. I suppose we don't really need xdg support during bootstrap, do you know how to replace this with a dummy version for bootstrap?

Thanks for taking care of this PR @emillon! Sorry for the lag, I was away on holiday...

rgrinberg · 2022-07-20T20:36:00Z

I suppose we don't really need xdg support during bootstrap, do you know how to replace this with a dummy version for bootstrap?

I don't think this is what the PR did, but we aren't allowed to do that. We should respect xdg settings when using the dune cache for example.

EDIT: the above doesn't even matter since the boostrapped binary is the one installed in the user's switch. So it has to be a fully functional dune.

emillon · 2022-07-21T08:18:57Z

the boostrapped binary is the one installed in the user's switch

oh is it? My knowledge about dune bootstrap dates from < 2.0 where we had a mini-dune. I assumed this was still the case, just with a different build strategy. Thanks for the clarification

emillon · 2022-07-21T08:19:28Z

Thanks for taking care of this PR @emillon! Sorry for the lag, I was away on holiday...

No problem!

anmonteiro · 2022-10-15T18:47:29Z

I'm seeing a Windows failure when trying to build current Dune master (with this PR).

Here's a GHA run: https://github.com/melange-re/melange/actions/runs/3256688824/jobs/5347432767

And relevant output:

      (cd _build/default/otherlibs/xdg && C:\npm\prefix\node_modules\esy\node_modules\esy-bash\.cygwin\bin\x86_64-w64-mingw32-gcc.exe -O2 -fno-strict-aliasing -fwrapv -mms-bitfields -g -I C:/Users/runneradmin/.esy/3__________________________________________________________/i/ocaml-4.14.0-cd77a8d9/lib/ocaml -o xdg_stubs.o -c xdg_stubs.c)
  
      xdg_stubs.c: In function 'dune_xdg__get_known_folder_path':
  
      xdg_stubs.c:35:9: warning: implicit declaration of function 'SHGetKnownFolderPath'; did you mean 'SHGetFolderPath'? [-Wimplicit-function-declaration]
  
         res = SHGetKnownFolderPath(rfid, 0, NULL, &wcp);
  
               ^~~~~~~~~~~~~~~~~~~~
  
               SHGetFolderPath
  
      xdg_stubs.c:41:38: error: 'WC_ERR_INVALID_CHARS' undeclared (first use in this function); did you mean 'MB_ERR_INVALID_CHARS'?
  
         len = WideCharToMultiByte(CP_UTF8, WC_ERR_INVALID_CHARS, wcp, wlen, NULL, 0, NULL, NULL);
  
                                            ^~~~~~~~~~~~~~~~~~~~
  
                                            MB_ERR_INVALID_CHARS
  
      xdg_stubs.c:41:38: note: each undeclared identifier is reported only once for each function it appears in
  
      error: command failed: "dune" "build" "-p" "xdg" "-j" "4" "@install" (exited with 1)

nojb · 2022-10-15T18:52:46Z

I'm seeing a Windows failure when trying to build current Dune master (with this PR).

This looks similar to #6098, which was supposedly fixed in #6109...

anmonteiro · 2022-10-15T18:54:22Z

@phated could you do a better test on your end on windows with esy? I'm using

  "resolutions": {
    "@opam/dune": "ocaml/dune:dune.opam#bc0a0c3169dfd4aae72498c5fc5b1859dd6f870e"
  }

rgrinberg · 2022-10-15T18:56:38Z

Made an issue for this in #6241

rgrinberg reviewed Jul 1, 2022

View reviewed changes

otherlibs/xdg/xdg_stubs.c Outdated Show resolved Hide resolved

rgrinberg reviewed Jul 1, 2022

View reviewed changes

otherlibs/xdg/xdg.ml Outdated Show resolved Hide resolved

rgrinberg added this to the 3.4.0 milestone Jul 1, 2022

emillon mentioned this pull request Jul 4, 2022

Move histfile to the system's "state" folder ocaml-community/utop#362

Closed

nojb force-pushed the xdg_windows branch 3 times, most recently from ce0b264 to 24a8071 Compare July 11, 2022 21:17

emillon force-pushed the xdg_windows branch from 9c27945 to ac117f9 Compare July 19, 2022 12:44

nojb added 7 commits July 20, 2022 14:21

Better XDG defaults for Windows

d571ef7

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

.gitattributes

7bcf2cb

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Add gen script

ddf4f57

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Fix

37032c3

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Review

ea12da1

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

make fmt

e87a849

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

CHANGES.md

b5cd276

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

nojb added 2 commits July 20, 2022 14:22

Fix

5d9c1b5

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

Fix

965f45c

Signed-off-by: nojebar <nicolas.ojeda.bar@lexifi.com>

emillon force-pushed the xdg_windows branch from ac117f9 to cba3bfc Compare July 20, 2022 12:25

Add -luuid

08c7999

Signed-off-by: Etienne Millon <me@emillon.org>

emillon force-pushed the xdg_windows branch from cba3bfc to 08c7999 Compare July 20, 2022 12:36

Merge branch 'main' into xdg_windows

7163e63

emillon merged commit 4b6261e into ocaml:main Jul 20, 2022

emillon mentioned this pull request Jul 20, 2022

[new release] xdg, stdune, ordering, ocamlc-loc, fiber, dyn, dune, dune-site, dune-rpc, dune-rpc-lwt, dune-private-libs, dune-glob, dune-configurator, dune-build-info, dune-action-plugin and chrome-trace (3.4.0) ocaml/opam-repository#21845

Merged

nojb deleted the xdg_windows branch July 20, 2022 19:10

kit-ty-kate mentioned this pull request Jul 25, 2022

Fix build on Cygwin/i686-w64-mingw32 #6008

Merged

rgrinberg mentioned this pull request Oct 15, 2022

I'm seeing a Windows failure when trying to build current Dune master (with this PR). #6241

Closed

rgrinberg added the windows label Dec 18, 2022

nojb mentioned this pull request Mar 20, 2023

Doc: fix default location of cache on Windows #7356

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better XDG defaults for Windows #5943

Better XDG defaults for Windows #5943

nojb commented Jul 1, 2022

rgrinberg commented Jul 1, 2022

jonahbeckford commented Jul 2, 2022

emillon commented Jul 4, 2022

zapashcanon commented Jul 5, 2022 •

edited

Loading

nojb commented Jul 5, 2022

zapashcanon commented Jul 5, 2022 •

edited

Loading

nojb commented Jul 5, 2022

dra27 commented Jul 5, 2022

nojb commented Jul 5, 2022 •

edited

Loading

nojb commented Jul 11, 2022

nojb commented Jul 11, 2022

nojb commented Jul 11, 2022

MisterDA commented Jul 15, 2022

emillon commented Jul 19, 2022

emillon commented Jul 19, 2022

nojb commented Jul 20, 2022

rgrinberg commented Jul 20, 2022 •

edited

Loading

emillon commented Jul 21, 2022

emillon commented Jul 21, 2022

anmonteiro commented Oct 15, 2022

nojb commented Oct 15, 2022

anmonteiro commented Oct 15, 2022 •

edited

Loading

rgrinberg commented Oct 15, 2022

Better XDG defaults for Windows #5943

Better XDG defaults for Windows #5943

Conversation

nojb commented Jul 1, 2022

rgrinberg commented Jul 1, 2022

jonahbeckford commented Jul 2, 2022

emillon commented Jul 4, 2022

zapashcanon commented Jul 5, 2022 • edited Loading

nojb commented Jul 5, 2022

zapashcanon commented Jul 5, 2022 • edited Loading

nojb commented Jul 5, 2022

dra27 commented Jul 5, 2022

nojb commented Jul 5, 2022 • edited Loading

nojb commented Jul 11, 2022

nojb commented Jul 11, 2022

nojb commented Jul 11, 2022

MisterDA commented Jul 15, 2022

emillon commented Jul 19, 2022

emillon commented Jul 19, 2022

nojb commented Jul 20, 2022

rgrinberg commented Jul 20, 2022 • edited Loading

emillon commented Jul 21, 2022

emillon commented Jul 21, 2022

anmonteiro commented Oct 15, 2022

nojb commented Oct 15, 2022

anmonteiro commented Oct 15, 2022 • edited Loading

rgrinberg commented Oct 15, 2022

zapashcanon commented Jul 5, 2022 •

edited

Loading

zapashcanon commented Jul 5, 2022 •

edited

Loading

nojb commented Jul 5, 2022 •

edited

Loading

rgrinberg commented Jul 20, 2022 •

edited

Loading

anmonteiro commented Oct 15, 2022 •

edited

Loading