Add more JSON #7894

marler8997 · 2018-02-15T04:57:31Z

Added fields to compilerInfo object in JSON object.

Note that this is based on #7521

It differs a bit, electing to go for a json representation matching the internals rather than trying to exactly match DUB probe files. Not sure which approach is better for DMD, this one is easier to implement so I went with this one for now.

Example Output:

{
 "compilerInfo" : {
  "vendor" : "Digital Mars D",
  "version" : "v2.078.2-742-gd6b8376-dirty",
  "__VERSION__" : 2078,
  "compilerInterface" : "dmd",
  "bitWidth" : 64,
  "platforms" : [
   "posix"
  ],
  "architectures" : [
   "x86_64"
  ],
  "predefinedVersions" : [],
  "supportsIncludeImports" : true
 },
 "buildInfo" : {
  "cwd" : "/home/marlerj/ddev1/dmd",
  "argv0" : "./generated/linux/release/64/dmd",
  "config" : "./generated/linux/release/64/dmd.conf",
  "importPaths" : [
   "./generated/linux/release/64/../../../../../druntime/import",
   "./generated/linux/release/64/../../../../../phobos"
  ],
  "objectFiles" : [],
  "libraryFiles" : [],
  "ddocFiles" : [],
  "mapFile" : null,
  "resourceFile" : null,
  "defFile" : null
 }
}

dlang-bot · 2018-02-15T04:57:32Z

Thanks for your pull request and interest in making D better, @marler8997! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

wilzbach

Wow that was quick! Thanks!


`compiler` and `frontendVersion` are a bit more effort, so it probably makes sense to do them in a quick followup (I can do this too).

Otherwise LGTM!

wilzbach · 2018-02-15T05:12:53Z

src/dmd/json.d

-        property("binary", global.params.argv0);
+        property("vendor", global.compiler.vendor);
        property("version", global._version);
+        property("argv0", global.params.argv0);


What's the motivation for renaming this?

Well, I was thinking that argv0 is more true to what it actually is. "binary" sounds more like a filename but argv0 can be an alias/string/filename, you know, whatever argv[0] happens to be. It may be helpful to include both "binary" and "argv0"...

wilzbach · 2018-02-15T05:14:56Z

src/dmd/json.d

+        property("vendor", global.compiler.vendor);
        property("version", global._version);
+        property("argv0", global.params.argv0);
+


compiler, frontendVersion are the interesting bits here.

Can you elaborate what frontendVersion and compilerFrontend are for?

(these variables are currently used by DUB in its platform probing)

frontendVersion: 2071, 2072, known in D as __VERSION__ (and in DMD as VERSIONX token). This is important for feature and compatibility detection

compiler, e.g. dmd, ldc or gdc - as mentioned the vendor might be different to the CLI interface (maybe compilerInterface is a better name)

compilerFrontend - same as version (already existent)

wilzbach · 2018-02-15T05:15:59Z

src/dmd/json.d

        objectStart();
-        property("binary", global.params.argv0);
+        property("vendor", global.compiler.vendor);
        property("version", global._version);


Probably a good idea to add DFLAGS too (see #7865), but I can easily add this in a follow-up PR.

inifilename is already in the buildInfo field

If we add DFLAGS...wouldn't we want to add all the command line arguments?

If we add DFLAGS...wouldn't we want to add all the command line arguments?

Yes, but again doesn't have to be in this PR.

wilzbach · 2018-02-15T05:25:34Z

Ah I see:

config is already part of buildInfo
version is already part of compilerInfo

So I think these would still be useful:

frontendVersion (e.g. 2071)
compilerInterface (e.g. dmd or ldc) - I think it's still good to keep this for dub In theory there could be a new vendor like "D Language Foundation" or someone else that ships or bundles DMD differently, but keeps the dmd interface.
DFLAGS (see Print DFLAGS on -v to ease debugging #7865)

But as mentioned, I don't expect this PR to include them.

timotheecour · 2018-02-15T06:31:29Z

definitely not PR blockers (could be done in future ones) but for reference:

json output shd probably be somewhere in docs (maybe in https://dlang.org/dmd-osx.html) ; real useful to have
linker flags
example with architectures.length > 1
predefinedVersions shouldn’t be empty (bug?)
stringImportPaths (on that note IIRC there was a bug where these would become absolute paths)

maybe:

“vendorShort” which is DMD instead of "Digital Mars D” (because VENDOR is "Digital Mars D” which is ugly and not matching other vendors (GDC and LDC iirc) and will just cause every single tool that reads vendor to add a logic to convert "Digital Mars D” to DMD
EDIT: i guess that's what @wilzbach referred to as compilerInterface ?

marler8997 · 2018-02-15T16:32:47Z

Ok, I think this is a good start. I've included "frontendVersion" as "__VERSION__" and also included "compilerInterface" as "interface". This should definitely have everything rdmd needs. I didn't add DFLAGS...I think I'll let @wilzbach figure out the best way to add that. Since this is experimental, if there's something wrong we can always fix it later.

Note for future changes. I opted for the compilerInfo/buildInfo objects to ALWAYS include fields even if their values are null/empty. This way a user can still see the format of the document even if a value is null (see resourceFile/defFile/etc as examples). However, to have these semantics with string fields, you need to use the propertyOrNull function instead of property. The property function will just leave the field out if the value is null.

marler8997 · 2018-02-15T16:39:10Z

NOTE: If you copy the following script to the test directory, you can run it after building DMD to automatically update the json tests with the new expected JSON output:

https://gist.github.com/marler8997/f3973cc6bcde7a88d04f48140ea8fe6b

## Script to update json test files
set -x

make test_results/compilable/json2.d.out
cp test_results/compilable/json2.out.sanitized compilable/extra-files/json2.out

make test_results/compilable/jsonCompilerInfo.d.out
cp test_results/compilable/jsonCompilerInfo.out.sanitized compilable/extra-files/jsonCompilerInfo.out

make test_results/compilable/json_nosource.sh.out
cp test_results/compilable/json_nosource.out.sanitized compilable/extra-files/json_nosource.out

wilzbach · 2018-02-16T01:16:18Z

src/dmd/json.d

+        propertyOrNull("version", global._version);
+        property("__VERSION__", global.versionNumber());
+        propertyOrNull("interface", determineCompilerInterface());
+        property("bitWidth", size_t.sizeof * 8);


Not sure if bitWidth is a good name.

yeah, any other ideas? It's the same number that's printed in the --version output.

size_tSizeof, or just size_t

ok, size_t seems reasonable. With size_t I also changed it to the "byte-width" instead of "bit-width". Let me know if someone prefers it to be the bit-width instead.

wilzbach · 2018-02-16T01:18:36Z

src/dmd/json.d

-                property("name", m.md.toChars());
-            property("file", m.srcfile.toChars());
+            propertyOrNull("name", m.md ? m.md.toChars() : null);
+            propertyOrNull("file", m.srcfile.toChars());


Can this even be null?

no, but it seemed like consistently using propertyOrNull was better. The new objects should use propertyOrNull and the old modules object uses property....maybe there is a better name....

The name requiredProperty came to mind and seems like a good idea.

wilzbach · 2018-02-16T01:20:20Z

src/dmd/json.d

+*/
+private const(char)* determineCompilerInterface()
+{
+    if (0 == strcmp(global.compiler.vendor, "Digital Mars D"))


Pattern is actual == expected (grep through the source code).
Of course !strcmp would work too.

wilzbach · 2018-02-16T01:22:32Z

src/dmd/globals.d

 alias StorageClass = uinteger_t;

 extern (C++) __gshared Global global;
+


wilzbach · 2018-02-16T01:24:05Z

src/dmd/json.d

+        propertyOrNull("cwd", getcwd(null, 0));
+        propertyOrNull("argv0", global.params.argv0);
+        propertyOrNull("config", global.inifilename);
+        propertyOrNull("libname", global.params.libname);


Everything else uses camelCase, might be a good idea to use libName too?
Precedent is above: we don't use inifilename, but config

WalterBright · 2018-02-19T21:54:12Z

src/dmd/globals.d

+    extern(C++) uint versionNumber()
+    {
+        import core.stdc.ctype;
+        static uint cached = 0;


Don't use thread local storage for dmd. Use __gshared

WalterBright · 2018-02-19T21:54:41Z

src/dmd/json.d

        comma();
    }

+    void propertyOrNull(const(char)* name, const(char)* s)


No Ddoc header

WalterBright · 2018-02-19T21:56:25Z

src/dmd/json.d

+            item("posix");
+            if (global.params.isLinux)
+                item("linux");
+            if (global.params.isOSX)


else if, etc.

WalterBright · 2018-02-20T00:44:21Z

src/dmd/globals.d

+    /**
+    Returns: the version as the number that would be returned for __VERSION__
+    */
+    extern(C++) uint versionNumber()


This code duplicates code in the lexer https://github.com/dlang/dmd/blob/master/src/dmd/lexer.d#L518

In fact, it's a cut&paste of it. It should take an argument instead of relying on a global, and moved to another module (lexer.d?).

As you can see later on in this PR, the code was removed from lexer.d and replaced with a call to this function. I could move this function to lexer.d and have json import lexer (instead of having json and lexer share the implementation from global). Would you prefer that? Although, I'm not sure if json importing lexer would cause a cyclic dependency issue...

Yes it could take an argument, but then the value won't be cached. This current implementation allows the value to be cached in case __VERSION__ is used multiple times along with outputing the __VERSION__ json value. Plus, there's currently no other caller who would pass something other than global.params._version.

I see you are correct. I missed that. I with draw both suggestions.

WalterBright · 2018-02-20T01:36:58Z

Is this no longer a WIP?

marler8997 · 2018-02-20T01:37:58Z

Correct no longer WIP. Though this does not "finalize" all the JSON fields...this should be ready for merge so others can build upon it.

wilzbach mentioned this pull request Feb 15, 2018

Implement -probe for showing supported features #7521

Closed

wilzbach reviewed Feb 15, 2018

View reviewed changes

marler8997 force-pushed the jsonFinalize branch 2 times, most recently from 2a52acd to e24c024 Compare February 15, 2018 05:44

marler8997 changed the title ~~Add JSON fields to compilerInfo~~ Add more JSON Feb 15, 2018

marler8997 force-pushed the jsonFinalize branch 2 times, most recently from 2447ad3 to 25a356b Compare February 15, 2018 06:03

marler8997 changed the title ~~Add more JSON~~ [WIP] Add more JSON Feb 15, 2018

dlang-bot added the Review:WIP Work In Progress - not ready for review or pulling label Feb 15, 2018

marler8997 force-pushed the jsonFinalize branch from 25a356b to d6b8376 Compare February 15, 2018 06:16

marler8997 force-pushed the jsonFinalize branch from d6b8376 to 8a40663 Compare February 15, 2018 16:19

marler8997 requested review from dnadlinger and ibuclaw as code owners February 15, 2018 16:19

marler8997 force-pushed the jsonFinalize branch from 8a40663 to 6cff412 Compare February 15, 2018 16:23

marler8997 changed the title ~~[WIP] Add more JSON~~ Add more JSON Feb 15, 2018

marler8997 force-pushed the jsonFinalize branch from 6cff412 to d5faa36 Compare February 15, 2018 16:36

marler8997 force-pushed the jsonFinalize branch 3 times, most recently from 04db990 to f81407c Compare February 15, 2018 17:23

wilzbach reviewed Feb 16, 2018

View reviewed changes

marler8997 force-pushed the jsonFinalize branch 2 times, most recently from 003a9e2 to 97b33c4 Compare February 18, 2018 05:52

WalterBright reviewed Feb 19, 2018

View reviewed changes

src/dmd/json.d Outdated

comma();

}

void propertyOrNull(const(char)* name, const(char)* s)

Copy link

Member

WalterBright Feb 19, 2018

Choose a reason for hiding this comment

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

No Ddoc header

WalterBright reviewed Feb 19, 2018

View reviewed changes

src/dmd/json.d Outdated

item("posix");

if (global.params.isLinux)

item("linux");

if (global.params.isOSX)

Copy link

Member

WalterBright Feb 19, 2018

Choose a reason for hiding this comment

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

else if, etc.

marler8997 force-pushed the jsonFinalize branch from 97b33c4 to caf1fcf Compare February 20, 2018 00:20

WalterBright reviewed Feb 20, 2018

View reviewed changes

Add JSON fields to compilerInfo

3c6cf52

marler8997 force-pushed the jsonFinalize branch from caf1fcf to 3c6cf52 Compare February 20, 2018 01:22

JinShil removed the Review:WIP Work In Progress - not ready for review or pulling label Feb 20, 2018

WalterBright added the Merge:auto-merge label Feb 20, 2018

dlang-bot merged commit d85a89c into dlang:master Feb 20, 2018

		alias StorageClass = uinteger_t;

		extern (C++) __gshared Global global;

Uh oh!

Add more JSON #7894

Add more JSON #7894

Uh oh!

Conversation

marler8997 commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Feb 15, 2018

Bugzilla references

Uh oh!

wilzbach left a comment • 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!

marler8997 Feb 15, 2018 • 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!

wilzbach Feb 15, 2018 • 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!

wilzbach Feb 15, 2018 • 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!

wilzbach commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timotheecour commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marler8997 commented Feb 15, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marler8997 commented Feb 15, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

WalterBright Feb 19, 2018 • 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!

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!

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!

marler8997 commented Feb 15, 2018 •

edited

Loading

wilzbach left a comment •

edited

Loading

marler8997 Feb 15, 2018 •

edited

Loading

wilzbach Feb 15, 2018 •

edited

Loading

wilzbach Feb 15, 2018 •

edited

Loading

wilzbach commented Feb 15, 2018 •

edited

Loading

timotheecour commented Feb 15, 2018 •

edited

Loading

marler8997 commented Feb 15, 2018 •

edited

Loading

WalterBright Feb 19, 2018 •

edited

Loading

marler8997 Feb 20, 2018 •

edited

Loading

marler8997 Feb 20, 2018 •

edited

Loading