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

.ocamlinit: XDG base directory lookup. #8834

Merged
merged 1 commit into from Sep 25, 2019
Merged

Conversation

dbuenzli
Copy link
Contributor

@dbuenzli dbuenzli commented Jul 25, 2019

This makes the ocaml tool lookup an .ocamlinit file in the configuration directory of the XDG base directory specification.

The file is ocaml/init.ml in the XDG configuration directory. This filename was chosen so that your editor recognizes it as a proper OCaml file. The XDG specification is followed by quite a few Linux distributions and its home directory fallback also makes it a good macos citizen.

The ~/.ocamlinit file is still looked up if the XDG lookup does not yield anything and on Windows this new lookup procedure is skipped, so no regressions should be expected.

On Windows %APPDATA% is tentatively looked up if XDG_CONFIG_HOME is undefined or empty. and a complicated fallback is implemented in case HOME is undefined and needed by the lookup procedure. One regression is that HOME is no longer looked up on Windows (as defined by Sys.win32, so not on cygwin) but %HomePath% is. This can be changed if Windows users would prefer no change in behaviour.

@dbuenzli
Copy link
Contributor Author

(In case anyone wonders about the code dupe this point 4. of #7589)

@dra27 dra27 self-requested a review July 26, 2019 16:12
if Sys.file_exists ocamlinit then Some ocamlinit else
let exists_in_dir dir p = match dir with
| None -> None
| Some dir ->
Copy link
Member

@Armael Armael Jul 27, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the weird (non-)indentation intentional? I would expect the match cases to be indented (and below as well)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the way ocp-indent idents for me (strict_with=always, a good way of making the most of the 80 columns that are available to express yourself) but I can certainly change that if it disturbs people.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact I realized there's an .ocp-indent in the repo but apparently it wasn't taken into account. Let me fix that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok it seems there was .emacs configuration taking over on my side. Now indented according the the repo's .ocp-indent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbuenzli What Emacs configuration? Tuareg indents | by default. It also indents the else clause:
https://github.com/ocaml/ocaml/pull/8834/files#diff-59d1589c8c3e651da23f92e75ce9c12eR550-R572

Copy link
Contributor Author

@dbuenzli dbuenzli Jul 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An ocp-indent emacs configuration. I don't use tuareg anyways but caml-mode.

@Armael
Copy link
Member

Armael commented Jul 27, 2019

I can't comment on the Windows-related changes, but the rest looks ok.

Since XDG is as far as I can tell a Linux(unix?)-specific thing, I would be slightly in favor of not breaking anything for Windows users... but to be fair I have zero insight on how HOME differs from %HomePath%.

@dbuenzli
Copy link
Contributor Author

Windows users... but to be fair I have zero insight on how HOME differs from %HomePath%.

AFAIK HOME simply doesn't exist on Windows so it's most likely unset. But it may be that exisiting OCaml Windows programmers started to set HOME in order for their .ocamlinit to be found. I could perfectly check both so that nothing is changed, I'll leave that call to windows users.

@nojb
Copy link
Contributor

nojb commented Jul 27, 2019

AFAIK HOME simply doesn't exist on Windows so it's most likely unset. But it may be that exisiting OCaml Windows programmers started to set HOME in order for their .ocamlinit to be found. I could perfectly check both so that nothing is changed, I'll leave that call to windows users.

I never used .ocamlinit myself, but HOME is set if you launch the toplevel from inside Cygwin so, yes, I think it is a good idea to check both.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Jul 27, 2019

I never used .ocamlinit myself, but HOME is set if you launch the toplevel from inside Cygwin so, yes, I think it is a good idea to check both.

But isn't Sys.win32 = false on cygwin ? EDIT yes it is so HOME is what will be looked up on cygwin.

@nojb
Copy link
Contributor

nojb commented Jul 28, 2019

But isn't Sys.win32 = false on cygwin ?

Actually, it depends which version of OCaml you are using. If you run the "native" ports (ie compiled with msvc or mingw compilers) under Cygwin (not an uncommon thing as far as I know), then Sys.win32 = true and HOME is set.

Put another way, cygwin is both a runtime and a development environment. The native Windows ports do not use cygwin as a runtime environment (and Sys.win32 = true) but many developers still use as a development environment (and HOME is defined as their home directory in Cygwin).

As another example of these kind of gymnastics, see #1406.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Jul 28, 2019

@nojb Thanks for the explanation. So the remaining question is: is in cygwin %HomePath% defined or not ? EDIT: and if both are to be looked up in which order windows users would like the lookup to happen.

@Chris00
Copy link
Member

Chris00 commented Jul 28, 2019 via email

@dra27
Copy link
Member

dra27 commented Jul 28, 2019

I’m fairly %HomePath% is wrong as I’m 99% sure it’s relative. I’ll do a proper review of the Windows side, but I’m not likely to manage that before Tuesday. The TL;DR is that I think HOME should be read first on Windows, or at least before other Windows-specific alternates.

@dbuenzli
Copy link
Contributor Author

Thanks @dra27 for your comment. So what do you think about determining HOME the way git does.

@dbuenzli dbuenzli force-pushed the ocaml-xdg branch 2 times, most recently from c85962d to d06482d Compare July 28, 2019 14:40
@dbuenzli
Copy link
Contributor Author

Since what I did was anyway stupid I went on to implement what git devs do to lookup HOME, presumably they have demanding Windows users. I hope this shall satisfy the Windows oracle when he reviews it.

This means HOME is now looked up again first on Windows, as it used to be. I didn't bother documenting the complicated lookup procedure in case HOME is undefined on Windows and simply referred to it as an "appropriate fallback".

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Libraries in other languages (Rust and Python) have put a lot of thought into the XDG + Windows side of this.

I don't like the Git approach - it's not uncommon, but it blats lots of unhidden dotfiles in a directory which is supposed to only contain other Microsoft-defined directories, and files blatted there do not roam.

Up to now on Windows, it's been necessary to set HOME to achieve this and I have indeed done that for as long as I can remember. I'd keep that part as no change on Windows - .ocamlinit is only found in the current directory or %HOME%.

I'm tempted to say don't check XDG_CONFIG_HOME at all on Windows, but for compatibility with running a native OCaml toplevel inside Cygwin, I guess it makes sense.

If only one of the APPDATA variables is to be checked, I'd check APPDATA (i.e. the roaming one). However, I think it would better to check %LOCALAPPDATA% and then %APPDATA%.

I think that eliminates the differences in home_dir and adds one further check for APPDATA in config_dir.

It's a nice change, thanks!

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

Incidentally, for the roaming/local thing on Windows, the idea is that machine-specific things go in %LOCALAPPDATA% and universally-global things go in %APPDATA% and it's the latter which may be either a redirected folder or is synchronised on logoff (or via Working folders).

For .ocamlinit, I don't think the toplevel can mandate whether it's machine-specific to all users, which is why I think it should check the local location first, and then the roaming location - it's then up to a user as to how they set those files up.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Jul 30, 2019

@dra27 thanks for your comments but I have difficulty understanding what and if I need to change something, could you please maybe make comments in the code what and where things should be changed (if any) ?

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

@dbuenzli - oops, sorry! I was suggesting:

  • Remove all the Windows-specific stuff in the home_dir function
  • In config_dir, check XDG_CONFIG_HOME then LOCALAPPDATA then APPDATA for ocaml/init.ml.

I hadn't spotted it before, but note that it is LOCALAPPDATA, not %LOCALAPPDATA% - %FOO% is just the cmd way of writing $FOO

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The environment change isn't quite what I meant - LOCALAPPDATA will always be defined on Windows. What I meant is if %LOCALAPPDATA%\ocaml\init.ml exists then use it, otherwise try %APPDATA%\ocaml\init.ml and otherwise fallback to the usual %HOME%\.ocamlinit.

Thinking some more, the same behaviour should apply to XDG_CONFIG_HOME, because the directory referred to may be a Cygwin, MSYS or WSL path, which native Windows will be unable to translate. So I think you'd want to try %XDG_CONFIG_HOME%\ocaml\init.ml and then if that doesn't exist (or XDG_CONFIG_HOME wasn't defined) then move on to %LOCALAPPDATA%, etc.)

I think that then means it's only a small tweak - instead of config_dir, you want config_dirs to return a list of possible directories to try (obviously, a user could choose to erase LOCALAPPDATA or APPDATA from their environment, so they shouldn't be tried at all if they're missing).

Changes Show resolved Hide resolved
toplevel/toploop.ml Outdated Show resolved Hide resolved
toplevel/opttoploop.ml Outdated Show resolved Hide resolved
@dbuenzli
Copy link
Contributor Author

I did what you suggested though it also changes the way I had implemented the XDG spec in a subtle way. Before you would determine a config directory and then lookup if a file exists in that directory.

Now you lookup a file in any of what config directory could be and load it if there is one in any of those. I don't think that's the way XDG is supposed to work but I think it's harmless for lookup.

However if you need to determine a directory for writing a configuration file that may not exist what would you suggest on windows ?

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Jul 30, 2019

I don't think that's the way XDG is supposed to work but I think it's harmless for lookup.

Well no I think it's problematic in fact.

If you set your XDG_HOME_CONFIG say to disable a set of configuration files you have in your ~/.config then with the new implementation it will catch your ~/.config/ocaml/init.ml which is certainly not what you want.

I think the behaviour on each platform should to be to take the first of the following files that exist:

  1. ./.ocamlinit
  2. Determine a suitable configuration directory $DIR. If $DIR/ocaml/init.ml exists use this.
  3. $HOME/.ocamlinit exists use this.

On non windows platforms $DIR of pt 2. should be determined according to XDG. Now @dra27 could you please tell me something sensitive to determine $DIR on windows ?

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

However if you need to determine a directory for writing a configuration file that may not exist what would you suggest on windows ?

As a slightly pedantic aside, when writing you'd want to adopt the Windows casing conventions (so the file would be OCaml\Init.ml). For whether it'd then go in APPDATA or LOCALAPPDATA that depends on exactly what the configuration data is - and that could mean you write and load two configuration files (that's certainly the route that Rust library has gone down).

For the XDG_CONFIG_HOME part and inter-operability between Cygwin/WSL builds of OCaml and native Windows, you'd probably just go ignore XDG_CONFIG_HOME for native Windows - the other schemes for that seem weird. But then running native Windows binaries within Cygwin or WSL is weird (as in the experience is weird, not the need to do so!)

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

On non windows platforms $DIR of pt 2. should be determined according to XDG. Now @dra27 could you please tell me something sensitive to determine $DIR on windows ?

For Windows, could it be predicated on the existence of the ocaml directory? So if %LOCALAPPDATA%\ocaml exists, then use it for $DIR. That means that an empty %LOCALAPPDATA%\ocaml directory (well, a directory not containing init.ml) overrides %APPDATA%\ocaml\init.ml?

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

@alainfrisch (and others who regularly use WSL builds of OCaml on Windows) - what do you think about whether Windows should always ignore %HOME%\.config\ocaml\init.ml?

@dbuenzli
Copy link
Contributor Author

For Windows, could it be predicated on the existence of the ocaml directory? So if %LOCALAPPDATA%\ocaml exists, then use it for $DIR. That means that an empty %LOCALAPPDATA%\ocaml directory (well, a directory not containing init.ml) overrides %APPDATA%\ocaml\init.ml?

I find the fact that this doesn't allow me to determine a location to write a non existing configuration file suspicious.

@dra27
Copy link
Member

dra27 commented Jul 30, 2019

For Windows, could it be predicated on the existence of the ocaml directory? So if %LOCALAPPDATA%\ocaml exists, then use it for $DIR. That means that an empty %LOCALAPPDATA%\ocaml directory (well, a directory not containing init.ml) overrides %APPDATA%\ocaml\init.ml?

I find the fact that this doesn't allow me to determine a location to write a non existing configuration file suspicious.

As far as I can tell (both knowing how Windows roaming configure "works", and looking at other libraries), it's because you're (well "one's") working under the assumption that there is one file and for Windows there's likely to be two. One of those libraries indeed adds a roaming parameter so the API is not just the name of the configuration file, it's also whether it's a file you expect to roam or not.

@dbuenzli
Copy link
Contributor Author

One of those libraries indeed adds a roaming parameter so the API is not just the name of the configuration file, it's also whether it's a file you expect to roam or not.

So I implemented the procedure I mentioned in this comment assuming OCaml Windows users want their .ocamlinit roaming which is consistent with the default of XDG_CONFIG_HOME which defaults in essence to a user directory, not a machine directory. These users who disagree can still define XDG_CONFIG_HOME if they think otherwise and or use $HOME/.ocamlinit or even the ./ocamlinit it seems to me there are sufficient escape hatches provided.

@dra27
Copy link
Member

dra27 commented Jul 31, 2019

I agree with the change to check APPDATA only.

Thinking more about invoking a native Windows binary from within another subsystem (since this is getting ever more popular), I'm uneasy about what happens when a native Windows ocaml reads an XDG_CONFIG_HOME value which almost certainly begins with /. It feels wrong that launching a native ocaml.exe from within WSL would fail to read %APPDATA%\OCaml\Init.ml because XDG_CONFIG_HOME has been correctly set for the WSL instance.

There's another issue in this situation with either XDG_CONFIG_HOME or HOME beginning with a /, which already applies to how ocaml processes HOME even without this PR. For native Windows, /home/dra is relative to the current disk, but not the current directory (so Filename.is_relative still returns false). So you could end up reading different files depending on the drive of the current working directory when ocaml was launched, which feels really wrong, if unlikely to happen in practice.

When Sys.win32 = true, would it be acceptable to ignore XDG_CONFIG_HOME (or HOME, as appropriate) if the first character of its value is / or \ (and, more strongly, also if Filename.is_relative actually returns true?)?

@dbuenzli
Copy link
Contributor Author

It feels wrong that launching a native ocaml.exe from within WSL would fail to read %APPDATA%\OCaml\Init.ml because XDG_CONFIG_HOME has been correctly set for the WSL instance.

(Note it will be %APPDATA%\ocaml\init.ml I really think it's better to have the same filenames across platforms here, especially if you are the user of multiple platforms, it makes it easier to sync things. I would certainly think otherwise for an end-user application but this is for developers)

@dra27 I already spent a bit too much time fiddling on this PR on Windows problems I do certainly not understand to their full extent. So maybe we could merge this and you can tweak the windows path yourself later ?

That being said

  1. I'm not sure I understand why the XDG_CONFIG_HOME of the WSL instance wouldn't match your %APPDATA% directory ? That's what I would expect from WSL if it is set to something no ?
  2. If setting HOME to /home/dra doesn't work on your platform maybe that's not the value you should set it to...
  3. What you propose at the end of the message seems super fiddly. In my experience this kind of "smart" things tend to bite users in the end and become with time usability nightmares. I think if you want to do this you should properly document it in the manuals.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Aug 6, 2019

Is there anything more that needs to be done for this to be merged ? The Windows support may not be perfect (see last two messages) but I don't think it is worse than before.

Copy link
Member

@dra27 dra27 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can go in if XDG_CONFIG_HOME is completely ignored for Sys.win32 = true (there'd be a man page tweak as well). It can read that in future if there's work to ensure the value makes sense.

toplevel/toploop.ml Show resolved Hide resolved
toplevel/opttoploop.ml Show resolved Hide resolved
@dbuenzli
Copy link
Contributor Author

dbuenzli commented Aug 7, 2019

It can go in if XDG_CONFIG_HOME is completely ignored for Sys.win32 = true (there'd be a man page tweak as well). It can read that in future if there's work to ensure the value makes sense.

I don't understand why you want this, I mean at that rate HOME shouldn't be consulted aswell on Windows, since what you said above also applies to HOME no ?

I'm fine not doing this if there's a good rationale for it, but for now there doesn't seem to be any. I don't see how it could harm windows user, it rather gives them a hook to configure things to their wishes and in a uniform way with other tools from the unix world that abide to that convention and they might be using.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Aug 7, 2019

uniform way with other tools from the unix world that abide to that convention and they might be using.

For example it seems git-for-windows respects it.

@dra27
Copy link
Member

dra27 commented Aug 7, 2019

Git-for-Windows is my example and reason for how not to do it - the interaction of Git-for-Windows in Cygwin is very easy to mess up, and is where it's coming from. There are even special tests in opam's CI to deal with Cygwin git vs Git-for-Windows git in the same environment.

My rationale is that if XDG_CONFIG_HOME has been set at all, it is highly likely it will not be a valid native Windows PATH, as I said in #8834 (comment), so in the absence of any further work, let's ignore it. Points 1 and 2 in #8834 (comment) appear to demonstrate that you don't use the platform (at least often), but there's no need to discuss it further if nothing's being done on the PR now.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Aug 7, 2019

My rationale is that if XDG_CONFIG_HOME has been set at all, it is highly likely it will not be a valid native Windows PATH,

Again, I don't see how this differs from the status of HOME but then: I no longer care.

Points 1 and 2 in #8834 (comment) appear to demonstrate that you don't use the platform (at least often),

I would rather say it is point 3 that demonstrates it: it seems every Windows user out there has a different opinion on how things should be done and they enjoy each of their tool to behave differently and/or in fiddly manners. Not my philosophy so I simply elected to restore the plain old behaviour for Windows so that nothing prevents this to be merged.

Thanks.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Aug 7, 2019

It seems there's a CI glitch here could someone please restart the job.

@dbuenzli
Copy link
Contributor Author

dbuenzli commented Sep 8, 2019

Ping. Is there something that prevents this to be merged ? The initial description is up-to-date with the current state of the PR.

@Armael
Copy link
Member

Armael commented Sep 8, 2019

@dra27 are you happy with the current state of the PR?
If yes then I'll happily merge the PR.

@dbuenzli
Copy link
Contributor Author

Ping. It seems to me that @dra27 should now have had enough time to comment on a PR that doesn't change anything as far as Windows is concerned.

@Armael
Copy link
Member

Armael commented Sep 25, 2019

Indeed. I'm merging the PR. Thank you for working on this!

@Armael Armael merged commit 17ef076 into ocaml:trunk Sep 25, 2019
@dbuenzli
Copy link
Contributor Author

Thanks @Armael !

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.

None yet

5 participants