Skip to content

json conf#2007

Closed
rtbo wants to merge 3 commits intoldc-developers:masterfrom
rtbo:json_conf
Closed

json conf#2007
rtbo wants to merge 3 commits intoldc-developers:masterfrom
rtbo:json_conf

Conversation

@rtbo
Copy link
Contributor

@rtbo rtbo commented Feb 18, 2017

Following work in #1832, I propose to eliminate the libconfig dependency by changing the config file format to JSON.

Advantages of JSON over libconfig:

  • format well known to everyone
  • use of a well tested and readily available parser
  • libconfig dependency!

Advantages of libconfig over JSON:

  • structuring features
  • typed fields
  • embedded comments

@rtbo
Copy link
Contributor Author

rtbo commented Feb 19, 2017

TravisCI failed due to networking issue.
Succesful build is here.

@rtbo
Copy link
Contributor Author

rtbo commented Feb 19, 2017

I realize that this is somehow difficult to review.
This is basically the same code as #1832, with only ConfigFile.readConfig adapted to use std.json instead of libconfig-d.
Of course .conf templates are moved to JSON (I kept some "comment" fields in there).

@kinke
Copy link
Member

kinke commented Feb 19, 2017

Thanks Remi for doing this JSON variant too. I think JSON is going to suffice for quite a while.

Do we have some policies wrt. usage of Phobos in the compiler itself? Afaik, it's already used by the LDC compilation cache and the math CTFE built-ins (starting with LDC v1.2). I guess the crucial aspect about Phobos is that we don't want a dependency on a certain (or at least rather recent) Phobos version.

@rtbo
Copy link
Contributor Author

rtbo commented Feb 19, 2017

std.json is already present in LDC-0.17.3 so I don't think it's going to be a problem for the bootstrap process.

@JohanEngelen
Copy link
Member

Do we have some policies wrt. usage of Phobos in the compiler itself?

I was thinking that perhaps we want to eradicate all Phobos usage, such that we can bootstrap without requiring Phobos. This could be a later task though, and I prefer to remove the libconfig dep sooner.

@dnadlinger
Copy link
Member

Not to rain on the parade here, but I'm not sure about the motivation for changing this in the first place. After all, it's not like we have any issues with libconfig right now, or dropping it would get rid of the last external dependency or something like that.

Yes, if I were to newly introduce a config file today, I would go with YAML/TOML/… (or possibly JSON, although it really isn't a good fit due to the lack of comments). But I'm not sure whether changing to an incompatible format without a pressing reason is ever going to be worth it – it would require everybody to change their setups, distro package build config, scripts and so on.

@kinke
Copy link
Member

kinke commented Feb 20, 2017

it's not like we have any issues with libconfig right now, or dropping it would get rid of the last external dependency or something like that.

Well, I hope they finally managed to fix their master, as AppVeyor and the release packages use older versions due to some bug I don't even remember OTOH. Their LGPL is responsible for half of the LDC license file. Having it as only external compilation and runtime dependency (libconfig-dev + libconfig) besides LLVM (and C++/D host compilers of course) is kind of ridiculous IMO for what we need it for - a lookup for an array of strings based on target triple.

I agree that JSON's lack for (standardized) comments is weak; it just came to mind as it's supported by Phobos out of the box. An easily embedded single-module YAML parser would be perfect.

If the config file format changes, we could make it less tiresome for distro maintainers etc. by combining it with the shared libs PR. They'll surely want to get rid of compiling the compiler twice and packaging manually, so reworking the scripts should pay off in such a case.

Edit: D-YAML seems rather huge, I may have underestimated YAML complexity. ;)

@JohanEngelen
Copy link
Member

  • I also hope we can remove the libconfig dependency (I remember it as something annoying when setting up my Windows build env, and also recently on my Pi3).
  • The config file is very easy and simple at this moment. We don't need complete support of any standard, I think an ad hoc parser will do fine as rainer proposed. I'd prefer that, and then we don't have to change the config format.
  • Let's try to come to an agreement and stick with that. @rtbo is spending a lot of time on this, and all the work that goes to waste is demotivating.

@timotheecour
Copy link

timotheecour commented Feb 20, 2017 via email

@rtbo
Copy link
Contributor Author

rtbo commented Feb 21, 2017

Let's try to come to an agreement and stick with that.

I cannot agree more :-)
I don't mind writting an adhoc parser, but I'll need clear (and if possible final) specifications.

  1. config file format
  • I think best to keep libconfig. No change in existing config file, and specification exists. I will however leave @include directives out of the parser.
  • If we'd need to change my vote would be TOML - just because I like it ;-)
  1. usage of phobos
  • I understand better without, so let's go without.
  1. usage of garbage collected memory
  • I've seen that driver starts by disabling garbage collection. Is it still OK to (sparingly) allocate GC memory? Or is it better to use malloc then free?
  1. language
  • I'd prefer to write in D, but I don't really see the benefit. Any votes for C++?

@kinke
Copy link
Member

kinke commented Feb 21, 2017

IMO:

  1. libconfig format; I'd even only implement the subset we currently need. It's absolutely unclear whether we'll ever require more than a serialized string[][string]. ;)
  2. better without Phobos
  3. I guess we're talking about a few kilobytes, so no need to resort to C malloc
  4. D if possible, C++ if more convenient (in case you could use something from LLVM or so, but that's rather unlikely)

@WebDrake
Copy link
Contributor

Not to rain on the parade here, but I'm not sure about the motivation for changing this in the first place. After all, it's not like we have any issues with libconfig right now, or dropping it would get rid of the last external dependency or something like that.

Agree. This is a very small part of LDC that works fine -- why change it?

I understand the wish to not say no to someone who's doing a lot of work, but it might be worth asking why it's important to put that work towards a goal like this. It sounds like it's adding a maintenance burden while bringing very little benefit.

(BTW, if there must be a change of format, I'd much rather YAML than JSON, both because of the markup format, and because std.json is really not very nice from what I recall.)

@rtbo

I've seen that driver starts by disabling garbage collection. Is it still OK to (sparingly) allocate GC memory?

What's the concern? Isn't this going to be a once-off parse-and-convert-to-an-internal-data-structure stage on program startup?

@kinke
Copy link
Member

kinke commented Feb 21, 2017

I understand the wish to not say no to someone who's doing a lot of work

It's absolutely not about that.

It sounds like it's adding a maintenance burden while bringing very little benefit.

I don't see any maintenance burden by a (hopefully small) integrated parser. I see simpler building (you only need CMake, make/ninja, host C++ and D compilers and LLVM; especially convenient on Windows, where libconfig has to be compiled externally, then passing paths around via CMake variables etc. - stuff that puts your regular Windows dev off), simpler redistribution (libconfig.so was missing from the LDC 1.0 OSX release package IIRC - and it's actually our only external runtime dependency if LLVM is linked statically) and 500 vs. 1000 lines in the license file.

@WebDrake
Copy link
Contributor

It's absolutely not about that.

It was raised as a point of concern, I understand it's not the only factor.

I don't see any maintenance burden by a (hopefully small) integrated parser.

It's the 'hopefully' that worries me ;-) But if there is a wish to move forwards with this, I would second your proposal to have a minimal parser to handle the existing config format.

@rtbo rtbo mentioned this pull request Feb 25, 2017
@kinke
Copy link
Member

kinke commented Mar 5, 2017

Closing in favor of #2016.

@kinke kinke closed this Mar 5, 2017
@rtbo rtbo deleted the json_conf branch March 7, 2017 20:43
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.

6 participants