Could you add a CHANGES entry?

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

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

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).

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)

%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 ?

%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?

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).

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 :)

Could you add a CHANGES entry?

Done.

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

The Windows CI is failing when building the bootstrap binary:

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

This comment, possibly related, suggests adding -luuid.

@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?

New error after bootstrap is:

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

@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...

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.

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

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

No problem!

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)
  

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...

@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"
  }

Made an issue for this in #6241