Add possibility to specify custom path to tzdata dir

[ThomasMader]

As mentioned in https://issues.dlang.org/show_bug.cgi?id=15391#c3 the hardcoded path to the tzdata directory does not work on NixOS.
To fix this I implemented a solution to provide the absolute path to tzdata via an import file. The path to the import file can be specified as parameter to make.

[dlang-bot]

Thanks for your pull request, @ThomasMader! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

• smaller, focused PRs are easier to review than big ones

• try not to mix up refactoring or style changes with bug fixes or feature enhancements

• provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

Bugzilla references

Auto-close	Bugzilla	Description
✓	15391	Problems loading libcurl.so and running datetime unittest on NixOS package build

[wilzbach]

Hmm, do we really need to hard-code the path? Wouldn't it be better to do the detection on the first time it's needed?

[wilzbach]

I know that not every parameter is documented, but we should still try to document new options/flags at the "readme" on top.

[ThomasMader]

For sure but is the README.md really the proper place for this option? It's too unimportant to get such a top level mention.
Maybe a new separate build helper file or something.

[ThomasMader]

Ah you talk about the readme at the top of the file. :-D
Will add some documentation.

[wilzbach]

Oh I meant the header of this Makefile: https://github.com/dlang/phobos/blob/master/posix.mak

As an example: I recently tried to cleanup the Makefile header of dlang.org and there's also this very informative header of DMD.

[ThomasMader]

Done

[wilzbach]

Hmm that doesn't look very elegant, but I'm also in lack of a better idea. Have you considered defining version=NixOS?

[ThomasMader]

NixOS is to specific I think. Guix will have the same problems at least.

[wilzbach]

But this would still require you to generate the file? How about generating/writing this file to $GENERATED directly in the Makefile? I guess that would save you one step and would make the docs easier as it's then just TZ_DATABASE_DIR what needs to be passed.

[ThomasMader]

Hmm, do we really need to hard-code the path? Wouldn't it be better to do the detection on the first time it's needed?

Don't think it's possible to detect the path to the dir.
AFAIK there is a tzdata lib, maybe the implementation should use this lib instead of parsing the files.
But that's quite some work I guess.

[PetarKirov]

Looks reasonable

[PetarKirov]

@jmdavis are you ok with this change?

[jmdavis]

Well, I find it very bizarre if the path isn't standardized on NixOS and have no clue how that could work, but clearly NixOS is doing some weird stuff. These changes don't seem to negatively impact anything outside of NixOS, so as long as they're appropriate for NixOS, I guess that they're okay.

[ThomasMader]

Well, I find it very bizarre if the path isn't standardized on NixOS and have no clue how that could work, but clearly NixOS is doing some weird stuff. These changes don't seem to negatively impact anything outside of NixOS, so as long as they're appropriate for NixOS, I guess that they're okay.

Reading my shitty explanation attempt in the issue I will try again to explain a little bit better.

In NixOS the package manager is called Nix and we should better talk about Nix because it is more correct because it is a platform dependend package manager. NixOS is just the main usage for Nix being a Linux distribution. Nix can be used on every other Linux distribution too side by side with the normal system without touching/affecting the base system in any way and can be used on OS X too.

In Nix all packages are installed under /nix/store/.
The path to the tzdata package can look like this: /nix/store/dvb915j007sqrvr5xiajm4jnp52kq96h-tzdata-2017c
So it consists of package name and version and begins with a hash value. Any time anything changes in the package of tzdata, the hash changes. This ensures that packages depending on this package always use this exact package.
Changing the tzdata package results in a rebuild of all dependent packages. If those packages still build with the changes done to the tzdata package, they all get a new hash for themselves too.
This way of managing packages ensures that a package build and tested on one machine, also works the same way on another because the dependencies and the package itself is frozen and never changes.
This results in many other advantages. Being able to declaratively setup machines or groups of machines and deploy them remotely and so on.

But one of the drawbacks is that every dependency needs to be addressed with it's full absolute path, otherwise it wouldn't be reproducible anymore.

Hope it's more clear now.

The issue with the libcurl.so in the issue is similar but I try to fix that by wrapping the resulting binary with a fixed LD_LIBRARY_PATH variable. This is quite common on Nix but not possible for the tzdata case because there is no env var.
Initially I tried to fix the problem by introducing an env var and read that in timezone.d but since getenv isn't working at compile time I went with the current solution.

[wilzbach]

• How about "path to the TZDatabase directory"?
• Also why is it named _FILE_DIR if it's a directory? Wouldn't TZ_DATABASE_DIR be less confusing? (see comment below about generating this file automatically).

[ThomasMader]

How about "path to the TZDatabase directory"?

TZ_DATABASE_FILE_DIR defines the path to the import file only. The path itself is in the file. I am not happy with the naming either but your proposal is not correct.

Also why is it named _FILE_DIR if it's a directory? Wouldn't TZ_DATABASE_DIR be less confusing? (see comment below about generating this file automatically).

Same as above.
It's the directory in which the TZ_DATABASE_FILE will be found in which the path is defined.
To make the naming more clear the import path and the TZ_DATABASE_DIR version flag can be separated.
The call to make will be more complicated this way because one needs to add IMPORT_PATH=path and something like TZ_DATABASE_DIR=1.
The naming would be much more clear though.

Or keep the current solution and go with TZ_DATABASE_IMPORT_FILE_DIR?

[wilzbach]

But this would still require you to generate the file? How about generating/writing this file to $GENERATED directly in the Makefile? I guess that would save you one step and would make the docs easier as it's then just TZ_DATABASE_DIR what needs to be passed.

[ThomasMader]

But this would still require you to generate the file? How about generating/writing this file to $GENERATED directly in the Makefile? I guess that would save you one step and would make the docs easier as it's then just TZ_DATABASE_DIR what needs to be passed.

Yes that's a good idea. Will change that.

[ThomasMader]

Yes that's a good idea. Will change that.

Done

[ThomasMader]

Regarding Auto-Tester by braddr:

wants to access your ThomasMader account
This application will be able to read and write all public repository data. This includes the following:

Code
Issues
Pull requests
Wikis
Settings
Webhooks and services
Deploy keys

Write access to my entire account is a little bit much to ask. Why is that needed at all?

[wilzbach]

Why is that needed at all?

Because the auto-tester used to be the only CI a couple of years ago and back then, you could login there with your GitHub account and toggle a PR for auto-merge. It then used your account for merging the PR. Nowadays we have dlang-bot which doesn't require any authentication as it's purely based on GitHub labels.
Anyhow, logging in at auto-tester is nowadays only useful to (a) restart a failing test and (b) approve new contributors for it.

[ThomasMader]

Anyhow, logging in at auto-tester is nowadays only useful to (a) restart a failing test and (b) approve new contributors for it.

Got it, thanks.

[wilzbach]

Thanks.
Final remark: Advantage of using $(ROOT)/TZDatabaseDirFile here would be that you will have a clean git status.

[ThomasMader]

I used /tmp instead because I don't want to put that file to the output too.

[wilzbach]

Well with /tmp you run into the problem of some people not having this directory as /tmp isn't available on all OS nor all Linux distributions, but we will rework our Makefile soon anyways and as long as you pass TZ_DATABASE_DIR as variable, everything will be working.

[ThomasMader]

What kind of rework?

[wilzbach]

The goal is to get rid of the ugly win{32,64}.mak files and unite these into one Makefile.

[wilzbach]

@ThomasMader I just saw that you haven't been approved for the auto-tester (unfortunately that CI still requires approval). I just approved you & this PR should now finally be tested on all CIs!