libconfig-d by rtbo · Pull Request #1832 · ldc-developers/ldc

rtbo · 2016-10-15T06:06:50Z

Hello

Pull-request according the project ideas page in order to remove dependency on libconfig.
Dependencies are managed by 2 submodules (libconfig-d and Pegged) in a 3rdparty directory, and necessary files are directly compiled in ldc2.o
So it should be transparent for users.

I setup Travis and AppVeyor to build this branch.

Travis reports error when building on OSX: dyld: Library not loaded: /opt/local/lib/libconfig.9.dylib during CMake configure step.
This dylib is not needed anymore (that's quite the purpose, isn't it?) but I can't tell where cmake is pulling this dependency from. Please give me indications.
In AppVeyor ldc emits warning that it can't find default.switches in the config file. I don't know how the config file looks like under windows, but I can remove the warning if it is normal. Otherwise please give me indication of what is wrong. Finally, windows build stops at this error: Error: module md is in file 'std\digest\md.d' which cannot be read. I don't think this is related to this PR, but can be wrong of course.

kinke · 2016-10-15T11:40:29Z

Very nice, thanks for doing this.

I can't tell where cmake is pulling this dependency from. Please give me indications.

dyld: Library not loaded: /opt/local/lib/libconfig.9.dylib
  Referenced from: /Users/travis/dlang/ldc-1.0.0/bin/ldmd2
  Reason: image not found

I.e., it's the used D host compiler (LDMD 1.0) which on OSX is apparently linked dynamically against libconfig.

In AppVeyor ldc emits warning that it can't find default.switches in the config file.

That's definitely not what's supposed to be happening. It's also the reason why the imports fail; no -I directories are set (only the ones directly in the command line used by CMake, i.e., -I${RUNTIME_DIR}/src -I${RUNTIME_DIR}/src/gc, Phobos is missing). The config file doesn't really differ on Windows. It's encoded in UTF8 without BOM afaik, but it uses Windows newlines (\r\n - probably done by git automatically). The path to it may use backslashes instead of forward ones on Windows. So I guess one of these 2 Windows specifica is causing the failure.

kinke · 2016-10-15T11:54:51Z

driver/configfile.cpp

+bool readDataFromConfigFile (   const char * pathcstr,
+                                const char * sectioncstr,
+                                const char * bindircstr,
+                                ConfigData & data);


Nitpick: please use clang-format for C++ sources.

kinke · 2016-10-15T12:09:05Z

driver/configfile.h


-  s_vector switches;
+  std::string pathstr;
+  ConfigData data;


How about getting rid of the ConfigData helper struct and implementing ConfigFile partially in D? Apparently all what would need to be done (besides having a D declaration of the (small) struct) is converting ConfigFile to a struct and storing pathstr as C string. readDataFromConfigFile() would become a private method implemented in D.

Yes, that's what I wanted to do at the start. I've chosen a safer path that modifies as little code as possible, but I prefer your option.
Any clue on how to call exe_path::getBinDir() from D? (it returns a std::string)

I'd let read() call it on the C++ side and forward its c_str() to the D method.

rtbo · 2016-10-15T14:24:09Z

For OSX, I'll reintegrate the brew install libconfig in .travis.yml. It should do the trick.

dnadlinger · 2016-10-15T15:12:43Z

Background: On OS X, the binary packages are supposed to contain libconfig, with the rpath suitably adapted. We really need to fix this for 1.0 and the current release, even if it will be obsoleted by this PR.

rationale: ldmd version used by travis links dynamically to libconfig

rtbo · 2016-10-16T22:48:23Z

It should work now.
AppVeyor still complains about a failed test (std.file-debug)

kinke · 2016-10-17T08:10:33Z

I've retriggered AppVeyor. Unfortunately, std.file sometimes fails due to 2-seconds file timestamp granularity (FAT file systems).

kinke · 2016-10-25T19:56:12Z

CMakeLists.txt

+    3rdparty/libconfig-d/src/config/package.d
+    3rdparty/libconfig-d/src/config/setting.d
+    3rdparty/libconfig-d/src/config/util.d
+)


In case those are all modules anyway, I'd prefer something compact and resilient like:

file(GLOB_RECURSE 3RDPARTY_SRC_D 3rdparty/pegged/*.d 3rdparty/libconfig-d/src/*.d )

Oh, I've just seen that you excluded libconfig-d's test.d in this list, but that's only enabled for unittest builds, so it doesn't really count. ;)

Edit: But pegged's pegged/test/tester.d contains a main(). Bad idea for library code; these testing helpers should be moved outside the regular library src tree IMO.

So I don't change it?
Or I glob and explicit remove afterwards what is not needed?
Pegged has a lot of files that we don't need at all, so I would prefer keep it this way.
Another possibility is to make a 3rdparty[.lib][.a] in 3rdparty/CMakeLists.txt

kinke · 2016-10-25T20:05:36Z

driver/configfile.h

-  s_vector switches;
+  const char *pathcstr;
+  s_iterator switches_b;
+  s_iterator switches_e;


Please initialize them to nullptr here in the header and get rid of the explicit default ctor.

kinke · 2016-10-25T20:07:25Z

driver/configfile.d

+module driver.configfile;
+
+
+struct ConfigFile


~~extern (C++) - otherwise all fields are off by ptr size (monitor for D classes).~~ Sorry it's a struct.

kinke · 2016-10-25T20:12:42Z

driver/configfile.d

+        extern(Windows)
+        HRESULT SHGetFolderPathA(HWND, int, HANDLE, DWORD, LPSTR);
+        extern(Windows)
+        HRESULT SHGetFolderPathW(HWND, int, HANDLE, DWORD, LPWSTR);


I'd prefer only declaring actually used stuff - no *W and only 3 of the enums below.

kinke · 2016-10-25T20:17:19Z

Are there any functional changes due to the C++ -> D transition of almost all of ConfigFile or did you take care to preserve existing behavior?

rtbo · 2016-10-26T06:50:05Z

Behavior (and C++ interface) should be the same.
I took care to locate ldc2.conf in the same order.

Avoids a glue C++ func that returns a C string

JohanEngelen · 2016-11-17T13:52:41Z

Is there anything still preventing this from merging?

kinke · 2016-11-17T16:12:31Z

I'd merge it if we agree that it should be part of 1.1, otherwise I'd wait (just to simplify bringing branch release-1.1 up to speed, i.e., one less commit to be reverted). If @klickverbot agrees that 1.1 should include all of master except for OSX (and the runtime functions type check, which depends on it). that is. If David is unsure whether other commits are responsible for the spurious Travis OSX failures, I'd tend to branch off a test branch (master with the 2 commits reverted) and trigger the OSX Travis jobs manually at least 20 times or so and see whether it's really just his OSX commit.

rainers · 2016-11-18T10:04:34Z

I'm a bit late to the discussion, but there is one thing that bugs me: while it is good that we change the implementation of the config file reading to something we can actively fix in case of existing and maybe future problems, it's a bit unfortunate to replace one external dependency with another.
Would it make sense to extract just the parser (e.g. no writing required) into a few files to include directly into the LDC source?

kinke · 2016-11-18T12:27:32Z

For me the primary advantages are:

no runtime dependency (as opposed to linking libconfig dynamically, which is at least the case for our previous OSX releases)
'built-in' compilation dependencies as submodules (e.g., on Linux we currently require the libconfig-dev package; on Windows the lib needs to be built externally and the path then passed to LDC's CMake etc.)
license

rainers · 2016-11-18T13:00:09Z

I agree, it's a good improvement. It was just a thought when I stumbled over some complaints that implementing an adhoc parser would be pretty simple, especially if we stretch the syntax slightly by reusing the D Lexer, e.g. for number parsing. But as I never got to do it, I must not complain about an alternative implementation ;-)

JohanEngelen · 2016-11-18T14:28:16Z

trigger the OSX Travis jobs manually at least 20 times or so and see whether it's really just his OSX commit

Currently doing this for #1817 (LLVM 3.9).

kinke · 2016-11-18T17:02:20Z

Currently doing this for #1817 (LLVM 3.9).

@JohanEngelen: I don't understand. That branch is 20 commits behind master and does contain David's OSX commit...

JohanEngelen · 2016-11-19T16:02:26Z

@kinke Should have checked that it contains the OSX stuff... dumb, sorry.
(edit: rebasing for extra testing)

JohanEngelen · 2017-01-10T08:51:26Z

Let's move forward on this?

JohanEngelen · 2017-02-05T11:08:14Z

Being on a constrained internet connection for some time made me more aware of the size of things. This PR pulls in 2 external submodules, and especially the Pegged repo is quite large and pulls in even more submodules. This will increase git clone time and download size and also our release source archive size, adding a lot of files that we don't need.
I now feel much more for @rainers suggestion of stripping all of it down to a few files. From the CMake script addition, it looks like we would only have to copy 9 .d files into our repo and two license files. (similar to how we include FileCheck and profile-rt, instead of svn checkout their repositories)

kinke · 2017-02-05T14:04:49Z

How about we just use a simple JSON file and parse it via std.json or so?

rtbo · 2017-02-07T00:11:08Z

FWIW:
Not that I want to scuttle my own work, but I tend to agree that JSON would be simpler to handle.
libconfig has nice structuring features, but all we need is to select a set of switches.

kinke · 2017-03-05T23:23:03Z

Closing in favor of #2016. Thanks Remi for hanging in there! :)

rtbo added 7 commits October 14, 2016 07:55

add libconfig-d 3rdparty modules

5e97a32

use libconfig-d instead of libconfig

631a656

remove libconfig from continous integration

4b7d722

config file templates libconfig doc

f15501e

Readme without libconfig

26206a3

License with libconfig-d and Pegged

fb99c4b

Merge branch 'master' into rtbo-libconfig

7964718

kinke reviewed Oct 15, 2016

View reviewed changes

rtbo added 6 commits October 15, 2016 19:26

keep libconfig installation on travis OSX

3bed990

rationale: ldmd version used by travis links dynamically to libconfig

clang-format configfile

a374b3b

readDataFromConfigFile => ConfigFile::readConfig

493c9d5

fix windows libconfig-d

549e3fd

ConfigFile::read implemented in D

bec2d9c

fix cmake syntax

749c9c7

ConfigFile.locate is also implemented in D

f54d4d1

This was referenced Oct 25, 2016

Add aarch64 support for 0.17 #1853

Closed

ldmd2 fails to find libconfig.dylib on macOS #1852

Closed

kinke reviewed Oct 25, 2016

View reviewed changes

rtbo added 2 commits October 26, 2016 08:34

remove explicit ConfigFile ctor

149e9c1

remove unused bindings from configfile.d

297c6af

rtbo added 2 commits October 26, 2016 17:04

driver/exe_path.d

8ae8204

configfile.d uses driver.exe_path

f91f6b4

Avoids a glue C++ func that returns a C string

kinke mentioned this pull request Nov 7, 2016

A new beta #1862

Closed

JohanEngelen mentioned this pull request Dec 20, 2016

build error: undefined reference to `llvm::Twine::str[abi:cxx11]() const' #1928

Closed

This was referenced Feb 18, 2017

json conf #2007

Closed

configuration parser #2016

Merged

kinke closed this Mar 5, 2017

rtbo deleted the rtbo-libconfig branch March 7, 2017 20:44

Uh oh!

Conversation

rtbo commented Oct 15, 2016

Uh oh!

kinke commented Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Oct 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtbo commented Oct 15, 2016

Uh oh!

dnadlinger commented Oct 15, 2016

Uh oh!

rtbo commented Oct 16, 2016

Uh oh!

kinke commented Oct 17, 2016

Uh oh!

kinke Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke Oct 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke commented Oct 25, 2016

Uh oh!

rtbo commented Oct 26, 2016

Uh oh!

JohanEngelen commented Nov 17, 2016

Uh oh!

kinke commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Nov 18, 2016

Uh oh!

kinke commented Nov 18, 2016

Uh oh!

rainers commented Nov 18, 2016

Uh oh!

JohanEngelen commented Nov 18, 2016

Uh oh!

kinke commented Nov 18, 2016

Uh oh!

JohanEngelen commented Nov 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JohanEngelen commented Jan 10, 2017

Uh oh!

JohanEngelen commented Feb 5, 2017

Uh oh!

kinke commented Feb 5, 2017

Uh oh!

rtbo commented Feb 7, 2017

Uh oh!

kinke commented Mar 5, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

kinke commented Oct 15, 2016 •

edited

Loading

kinke Oct 15, 2016 •

edited

Loading

kinke Oct 25, 2016 •

edited

Loading

kinke Oct 25, 2016 •

edited

Loading

kinke commented Nov 17, 2016 •

edited

Loading

JohanEngelen commented Nov 19, 2016 •

edited

Loading