timewarrior icon indicating copy to clipboard operation
timewarrior copied to clipboard

Add support for XDG Base Directory specification on Unixes

Open fenuks opened this issue 2 years ago • 11 comments

This adds support for XDG Base Specification on Unix systems (MacOS not included). This fixes https://github.com/GothenburgBitFactory/timewarrior/issues/207.

I'll update documentation when final shape of changes will be agreed upon. For now:

  • if $TIMEWARRIORDB exists, store all data there
  • if ~/.timewarrior directory exists, and $TIMEWARRIORDB is not defined, store all data there
  • otherwise, store config and extensions in $XDG_CONFIG_HOME/timewarrior; and data in $XDG_DATA_HOME/timewarrior

There is no automatic migration, and I think there shouldn't be. This allows people who dislike XDG BD, to use single directory layout (in fact, they could achieve the same with setting XDG_CONFIG_HOME and XDG_DATA_HOME appropriately, but that would require setting shell alias).

fenuks avatar Jan 27 '22 16:01 fenuks

@fenuks Thank you for your contribution. I will try to have a closer look at it over the weekend. Please do not forget to sign off your commits according to the 👉🏻 DCO

Btw. I think there is a typo in your listing of behaviour:

  • ...
  • if ~/.taskwarrior directory exists, and $TIMEWARRIORDB is not defined, store all data there
  • otherwise...

I guess you mean ~/.timewarrior, right?

lauft avatar Jan 28 '22 06:01 lauft

Yes, I meant ~/.timewarrior.

fenuks avatar Jan 28 '22 08:01 fenuks

Hi @fenuks!

  • if $TIMEWARRIORDB exists, store all data there
  • if ~/.timewarrior directory exists, and $TIMEWARRIORDB is not defined, store all data there
  • otherwise, store config and extensions in $XDG_CONFIG_HOME/timewarrior; and data in $XDG_DATA_HOME/timewarrior

ok. The final XDG paths willl then be

  • $XDG_CONFIG_HOME/timewarrior/ for timewarrior.cfg (legacy ~/.timewarrior/)
  • $XDG_CONFIG_HOME/timewarrior/extensions/ for extensions (legacy ~/.timewarrior/extensions/)
  • $XDG_DATA_HOME/timewarrior/ for tags.data, undo.data, and the monthly *.data files (legacy ~/.timewarrior/)

Btw., did you consider the $XDG_*_DIRS variables? In my opinion those should not be supported. This is not relevant for the code, but for the documentation.

There is no automatic migration, and I think there shouldn't be. ...

Agreed. If it lets existing users (with ~/.timewarrior) use their setup as-is, then this is fine.

In any way, the user should only be queried once if a new setup is created. For the new setup something like the following could be displayed:

Configuration will be stored in <config directory>.
Data will be stored in <data directory>.
Proceed? (yes/no)

Your proposed change made me think a lot about the current way how those folders and the elements depending on them are defined and initialized. This change may be the chance to do some refactoring as well. Some ideas:

  • encapsulate the detection and creation of the directory paths into a single class which has as interface like
    PathResolver::getConfigDir()
    PathResolver::getDataDir() 
    PathResolver::getConfigDir()
    PathResolver::isLegacy()
    
    all handling of environment variables/probing for existing directories will happen in there.
  • Move the actual creation of the paths into the respective elements, i.e. database, journal, ...
  • disentangle the function initializeDataJournalAndRules and give database, journal, and rules each a dedicated init function.

lauft avatar Feb 03 '22 22:02 lauft

Hello @lauft,

ok. The final XDG paths willl then be

* `$XDG_CONFIG_HOME/timewarrior/` for `timewarrior.cfg` (legacy `~/.timewarrior/`)

* `$XDG_CONFIG_HOME/timewarrior/extensions/` for extensions (legacy `~/.timewarrior/extensions/`)

* `$XDG_DATA_HOME/timewarrior/` for `tags.data`, `undo.data`, and the monthly `*.data` files (legacy `~/.timewarrior/`)

Yes, except that for data directory is $XDG_DATA_HOME/timewarrior/data. data directory can be stripped, but I suppose it might be better to keep it, just in case you would like in the future to keep something else there as well.

Btw., did you consider the $XDG_*_DIRS variables? In my opinion those should not be supported. This is not relevant for the code, but for the documentation.

You mean $XDG_DESKTOP_DIR, $XDG_DOCUMENTS_DIR and the rest of the family? No, I didn't, it's uncommon for Linux apps to keep data in user directories.

There is no automatic migration, and I think there shouldn't be. ...

Agreed. If it lets existing users (with ~/.timewarrior) use their setup as-is, then this is fine.

I can write shell script that will move directories accordingly to ease burden of migration for those interested, but I think it should be executed manually.

In any way, the user should only be queried once if a new setup is created. For the new setup something like the following could be displayed:

Configuration will be stored in <config directory>.
Data will be stored in <data directory>.
Proceed? (yes/no)

Alright, I will improve the code.

Your proposed change made me think a lot about the current way how those folders and the elements depending on them are defined and initialized. This change may be the chance to do some refactoring as well. Some ideas:

* encapsulate the detection and creation of the directory paths into a single class which has as interface like
  ```
  PathResolver::getConfigDir()
  PathResolver::getDataDir() 
  PathResolver::getConfigDir()
  PathResolver::isLegacy()
  ```
  all handling of environment variables/probing for existing directories will happen in there.

* Move the actual creation of the paths into the respective elements, i.e. database, journal, ...

* disentangle the function `initializeDataJournalAndRules` and give database, journal, and rules each a dedicated init function.

I will try that, but keep in mind I don't know the codebase well, nor I use C++ for my daily work, so I wonder if my execution will be fine. With current changeset, I tried to keep modifications as minimal as possible, with all pluses and minuses of this approach.

fenuks avatar Feb 04 '22 08:02 fenuks

Yes, except that for data directory is $XDG_DATA_HOME/timewarrior/data.

Right. (I mistyped this above...)

You mean $XDG_DESKTOP_DIR, $XDG_DOCUMENTS_DIR and the rest of the family?

I meant XDG_DATA_DIRS and XDG_CONFIG_DIRS which are used to define additional directories for the respective XDG..._HOME directory. (see https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html#variables)

Your proposed change made me think a lot about the current way how those folders and the elements depending on them are defined and initialized. This change may be the chance to do some refactoring as well. Some ideas:

I will try that, but keep in mind I don't know the codebase well, nor I use C++ for my daily work, so I wonder if my execution will be fine. With current changeset, I tried to keep modifications as minimal as possible, with all pluses and minuses of this approach.

Proceed at your pace. 👍🏻 I can also implement some of the suggested ideas later, but I wanted to give you the first move here.

However, for your changes to be accepted, you will have to agree to the DCO by signing your commits accordingly.

lauft avatar Feb 04 '22 19:02 lauft

I meant XDG_DATA_DIRS and XDG_CONFIG_DIRS which are used to define additional directories for the respective XDG..._HOME directory. (see https://specifications.freedesktop.org/basedir-spec/basedir-spec-latest.html#variables)

Oh, I see. It's been so many years since I read the spec, that I completely forgotten about this one. To my understanding, it would be useful to support e.g. if timewarrior had system-wide read-only config that would be expected at one of XDG_CONFIG_DIRS. It seems that timewarrior doesn't need this. My installation on Arch Linux has only binaries, documentation and manual pages.

I wonder if many applications actually support this. I moved dozens app configurations to XDG Base Directory compliant locations, and I didn't see any mentions of this in manual pages.

Proceed at your pace. 👍🏻 I can also implement some of the suggested ideas later, but I wanted to give you the first move here.

I can at least try. If I for some reason will not be able to do that, then I simply let you know, and you will proceed with refactoring.

However, for your changes to be accepted, you will have to agree to the DCO by signing your commits accordingly.

Yes, I will. I expected that some changes will be necessary, and will have to amend commit anyway.

fenuks avatar Feb 05 '22 09:02 fenuks

I've refactored code as suggested. This has not been extensively tested yet, but seems to work. I'd like to get your early feedback, please tell me if this approach overall seems fine to you, or you have something different in mind.

BTW, I've seen your FOSSDEM talk. You did good job explaining basics of the tool, I think.

fenuks avatar Feb 13 '22 18:02 fenuks

I've refactored code as suggested. This has not been extensively tested yet, but seems to work. I'd like to get your early feedback, please tell me if this approach overall seems fine to you, or you have something different in mind.

I will try to look at it as soon as possible, but it may take until next weekend.

BTW, I've seen your FOSSDEM talk. You did good job explaining basics of the tool, I think.

Thanks

lauft avatar Feb 14 '22 22:02 lauft

Thanks for the review. Other than your requested changes, failing tests, and documentation changes that are still missing – is there anything else I should keep in mind?

fenuks avatar Feb 21 '22 09:02 fenuks

Other than your requested changes, failing tests, and documentation changes that are still missing – is there anything else I should keep in mind?

At the moment I have nothing to add here. 👍🏻

The current failing test is because of CentOS 8 being discontinued. I am currently deciding wether to remove CentOS from the test zoo or replace it.

lauft avatar Feb 21 '22 23:02 lauft

Sorry for late entry. Much has happened at the end of February and I simply forgotten about this review. I've updated the code, hopefully resolving all problems, and added documentation detailing XDG support.

fenuks avatar Apr 04 '22 18:04 fenuks

@fenuks I had to merge the branch manually because of conflicts. But your contribution is now on develop. Thanks a lot! ❤️

lauft avatar Oct 16 '22 09:10 lauft

Thank you very much for taking care of those. :) Glad to see this merged.

fenuks avatar Oct 17 '22 08:10 fenuks

What should those Macos' user which make use of XDG variables should do to keep the home clean?

ninoCan avatar Dec 14 '22 18:12 ninoCan

XDG support is enabled compile-time, therefore if you can compile timewarrior yourself, add check for MacOS here (MacOS doesn't define unix), and it should work for you.

fenuks avatar Dec 14 '22 22:12 fenuks

Why on earth was macOS excluded? I understand that most compilers on macOS do not define the __unix__ macro, but that does not mean that macOS is not a UNIX based system. In fact this is more of a compiler issue and does not have much to do with the OS. The documentation is plainly wrong when stating that macOS is non-unix system (sure, it has branched off of its UNIX roots, but so has Linux!). As far as I understand it adding support for XDG specification for macOS is just a matter of changing the macro check from #ifdef __unix__ to something more comprehensive like #if defined(__unix__) || defined(__APPLE__) || defined(__linux__) || defined(BSD) as suggested here

Rahlir avatar Jul 25 '23 11:07 Rahlir