Skip to content

Add more stuff to JSON file #7757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wilzbach merged 1 commit into from
Feb 2, 2018
Merged

Add more stuff to JSON file #7757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 37 additions & 0 deletions changelog/more_json.dd
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
Added more information to the json output

The following objects were to the json output:
---
[
{
"kind" : "compilerInfo",
"binary" : "<filename-of-compiler-binary>",
"version" : "<compiler-version>",
"supportsIncludeImports" : true
},
{
"kind" : "buildInfo",
"config": "<filename-of-config-file>",
"cwd": "<cwd-during-compiler-invocation>",
"importPaths": [
"<import-path1>",
"<import-path2>",
//...
]
},
// ...
{
"kind": "semantics",
"modules": [
{
"name": "<module-name>"
"file": "<module-filename>",
// isRoot is true if the module is going to be taken
// to object code.
"isRoot": true|false,
},
// ...
]
}
]
---
1 change: 1 addition & 0 deletions src/dmd/dmodule.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ extern (C++) final class Module : Package
uint numlines; // number of lines in source file
int isDocFile; // if it is a documentation input file, not D source
bool isPackageFile; // if it is a package.d
Strings contentImportedFiles; // array of files whose content was imported
int needmoduleinfo;
/**
How many unit tests have been seen so far in this module. Makes it so the
Expand Down
1 change: 1 addition & 0 deletions src/dmd/expressionsem.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4282,6 +4282,7 @@ private extern (C++) final class ExpressionSemanticVisitor : Visitor
return setError();
}

sc._module.contentImportedFiles.push(name);
if (global.params.verbose)
fprintf(global.stdmsg, "file %.*s\t(%s)\n", cast(int)se.len, se.string, name);
if (global.params.moduleDeps !is null)
Expand Down
65 changes: 65 additions & 0 deletions src/dmd/json.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ import dmd.mtype;
import dmd.root.outbuffer;
import dmd.visitor;

version(Windows) {
extern (C) char* getcwd(char* buffer, size_t maxlen);
Copy link
Contributor

@timotheecour timotheecour Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

is that a concern? what does other version(Windows) code do in other D files?
https://msdn.microsoft.com/en-us/library/ms235450.aspx
This POSIX function is deprecated. Use the ISO C++ conformant _getcwd instead.

Copy link
Contributor Author

@marler8997 marler8997 Jan 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This declaration was copied from dmodule.d. If it's a problem then the fix should be a separate PR.

Copy link
Contributor

@wilzbach wilzbach Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Always open an issue about such stuff, otherwise it will be forgotten.
I just did so for this: https://issues.dlang.org/show_bug.cgi?id=18291

} else {
import core.sys.posix.unistd : getcwd;
}

private extern (C++) final class ToJsonVisitor : Visitor
{
alias visit = Visitor.visit;
Expand Down Expand Up @@ -789,19 +795,78 @@ public:
jsonProperties(d);
objectEnd();
}

private void generateCompilerInfo()
{
objectStart();
property("kind", "compilerInfo");
property("binary", global.params.argv0);
property("version", global._version);
propertyBool("supportsIncludeImports", true);
objectEnd();
}
private void generateBuildInfo()
{
objectStart();
property("kind", "buildInfo");
property("cwd", getcwd(null, 0));
property("config", global.inifilename ? global.inifilename : null);
if (global.params.lib) {
property("library", global.params.libname);
}
propertyStart("importPaths");
arrayStart();
foreach (importPath; *global.params.imppath)
{
item(importPath);
}
arrayEnd();
objectEnd();
}
private void generateSemantics()
{
objectStart();
property("kind", "semantics");
propertyStart("modules");
arrayStart();
foreach (m; Module.amodules)
{
objectStart();
if(m.md)
property("name", m.md.toChars());
Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

how about:
property("name", m.md ? m.md.toChars() : "");
to make each entry have each field? (and update https://github.com/dlang/tools/pull/292/files#diff-e6f94050037e7c9a64684829764b1f0bR832 accordingly)

Copy link
Contributor Author

@marler8997 marler8997 Jan 22, 2018

Choose a reason for hiding this comment

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

I was following the convention established here (https://github.com/dlang/dmd/pull/7757/files/a90c983d6e436e46dde3b1ee07fdd280ea1396e7#diff-1d73c979f233c43e47fb88fac32c21b3L475)

I don't really see much difference whether or not there is an empty name or no name at all, but following the existing convention seems the better choice.

property("file", m.srcfile.toChars());
Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018

Choose a reason for hiding this comment

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

how about absolutePath ( m.srcfile.toChars() ) so that if rdmd is called from a different directory, it'll still work?

Copy link
Contributor Author

@marler8997 marler8997 Jan 22, 2018

Choose a reason for hiding this comment

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

hmmmm, interesting thought. I'd have to think about that one. This is mimicking the current functionality from the verbose output, but maybe it can be improved. I'll come back to this.

Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018

Choose a reason for hiding this comment

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

NOTE: "contentImports" appears to use absolutePath (i tried with -J. and import("foo.txt") ), so I would make sense to be consistent. I suggest using absolutePath for file as well (and wherever else it makes sense)

Copy link
Contributor Author

@marler8997 marler8997 Jan 22, 2018

Choose a reason for hiding this comment

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

I believe that would be changing behavior though...we could end up with a regression if we change the behavior.

Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

well, that info wasn't in json output before... so it's one of those gray area things.
one option is to allow configuring json output (as I suggested somewhere else), which is OK if we have good (easily extensible) way to customize, eg:

-Xf=file -Xopt=abspaths,imports,-functions
each option has a corresponding - flag to negate it, just like you did for -i=core,-std

Copy link
Contributor Author

@marler8997 marler8997 Jan 22, 2018

Choose a reason for hiding this comment

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

Now that I think about it, there might be a bug in the current design. If you build with rdmd, and then you change directories and build again, then all the relative module filenames will be incorrect. It might just force you to do a rebuild, but that's no good. We could fix this using absolute paths, or we could save off the original CWD that rdmd was using when it first performed the build.

Copy link
Member

@CyberShadow CyberShadow Jan 22, 2018

Choose a reason for hiding this comment

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

There should be a test for that in the rdmd test suite, FWIW.

Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018

Choose a reason for hiding this comment

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

there may even be corner cases where it picks up wrong file eg:

ls
main.d
fun.d
temp/fun.d // slightly different version main.d

rdmd main.d
cd temp
rdmd ../main.d
=> can it get confused and get the wrong fun.d?

Here's a good compromise:

add a CWD field, and when rdmd reads a path (any path) call buildPath(json.CWD, json.path) (does the right thing w absolute paths)

and allow for:
rdmd main.d
cd temp
rdmd ../main.d => should reuse cache and not rebuild

Copy link
Contributor

@timotheecour timotheecour Jan 22, 2018

Choose a reason for hiding this comment

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

but we should definitely make behavior consistent between contentImports and modules.file:
import("fun.txt") should not get auto-transformed in an absolute path either.

Copy link
Contributor Author

@marler8997 marler8997 Jan 22, 2018

Choose a reason for hiding this comment

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

I'm thinking that the JSON file saves the CWD in the new buildInfo object.

propertyBool("isRoot", m.isRoot());
if(m.contentImportedFiles.dim > 0)
{
propertyStart("contentImports");
arrayStart();
foreach (file; m.contentImportedFiles)
{
item(file);
}
Copy link
Contributor

@wilzbach wilzbach Feb 2, 2018

Choose a reason for hiding this comment

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

This isn't covered.

arrayEnd();
}
objectEnd();
}
arrayEnd();
objectEnd();
}
}

extern (C++) void json_generate(OutBuffer* buf, Modules* modules)
{
scope ToJsonVisitor json = new ToJsonVisitor(buf);
json.arrayStart();
json.generateCompilerInfo();
json.generateBuildInfo();
for (size_t i = 0; i < modules.dim; i++)
{
Module m = (*modules)[i];
if (global.params.verbose)
fprintf(global.stdmsg, "json gen %s\n", m.toChars());
m.accept(json);
}
json.generateSemantics();
json.arrayEnd();
json.removeComma();
}
1 change: 1 addition & 0 deletions src/dmd/module.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class Module : public Package
unsigned numlines; // number of lines in source file
int isDocFile; // if it is a documentation input file, not D source
bool isPackageFile; // if it is a package.d
Strings contentImportedFiles; // array of files whose content was imported
int needmoduleinfo;
/**
How many unit tests have been seen so far in this module. Makes it so the
Expand Down
27 changes: 18 additions & 9 deletions test/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,24 +203,26 @@ fail_compilation_test_results=$(addsuffix .out,$(addprefix $(RESULTS_DIR)/,$(fai

all: run_tests

$(RESULTS_DIR)/runnable/%.d.out: runnable/%.d $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
test_tools: $(RESULTS_DIR)/d_do_test$(EXE) $(RESULTS_DIR)/sanitize_json$(EXE)

$(RESULTS_DIR)/runnable/%.d.out: runnable/%.d $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) $(RESULTS_DIR)/d_do_test $(<D) $* d

$(RESULTS_DIR)/runnable/%.sh.out: runnable/%.sh $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
$(RESULTS_DIR)/runnable/%.sh.out: runnable/%.sh $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) echo " ... $(<D)/$*.sh"
$(QUIET) ./$(<D)/$*.sh

$(RESULTS_DIR)/compilable/%.d.out: compilable/%.d $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
$(RESULTS_DIR)/compilable/%.d.out: compilable/%.d $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) $(RESULTS_DIR)/d_do_test $(<D) $* d

$(RESULTS_DIR)/compilable/%.sh.out: compilable/%.sh $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
$(RESULTS_DIR)/compilable/%.sh.out: compilable/%.sh $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) echo " ... $(<D)/$*.sh"
$(QUIET) ./$(<D)/$*.sh

$(RESULTS_DIR)/fail_compilation/%.d.out: fail_compilation/%.d $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
$(RESULTS_DIR)/fail_compilation/%.d.out: fail_compilation/%.d $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) $(RESULTS_DIR)/d_do_test $(<D) $* d

$(RESULTS_DIR)/fail_compilation/%.html.out: fail_compilation/%.html $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE) $(DMD)
$(RESULTS_DIR)/fail_compilation/%.html.out: fail_compilation/%.html $(RESULTS_DIR)/.created test_tools $(DMD)
$(QUIET) $(RESULTS_DIR)/d_do_test $(<D) $* html

quick:
Expand All @@ -242,19 +244,19 @@ run_tests: start_runnable_tests start_compilable_tests start_fail_compilation_te

run_runnable_tests: $(runnable_test_results)

start_runnable_tests: $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE)
start_runnable_tests: $(RESULTS_DIR)/.created test_tools
@echo "Running runnable tests"
$(QUIET)$(MAKE) --no-print-directory run_runnable_tests

run_compilable_tests: $(compilable_test_results)

start_compilable_tests: $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE)
start_compilable_tests: $(RESULTS_DIR)/.created test_tools
@echo "Running compilable tests"
$(QUIET)$(MAKE) --no-print-directory run_compilable_tests

run_fail_compilation_tests: $(fail_compilation_test_results)

start_fail_compilation_tests: $(RESULTS_DIR)/.created $(RESULTS_DIR)/d_do_test$(EXE)
start_fail_compilation_tests: $(RESULTS_DIR)/.created test_tools
@echo "Running fail compilation tests"
$(QUIET)$(MAKE) --no-print-directory run_fail_compilation_tests

Expand All @@ -266,3 +268,10 @@ $(RESULTS_DIR)/d_do_test$(EXE): d_do_test.d $(RESULTS_DIR)/.created
$(DMD) -conf= $(MODEL_FLAG) $(DEBUG_FLAGS) -unittest -run d_do_test.d -unittest
$(DMD) -conf= $(MODEL_FLAG) $(DEBUG_FLAGS) -od$(RESULTS_DIR) -of$(RESULTS_DIR)$(DSEP)d_do_test$(EXE) d_do_test.d

$(RESULTS_DIR)/sanitize_json$(EXE): sanitize_json.d $(RESULTS_DIR)/.created
@echo "Building sanitize_json tool"
@echo "OS: '$(OS)'"
@echo "MODEL: '$(MODEL)'"
@echo "PIC: '$(PIC_FLAG)'"
$(DMD) -conf= $(MODEL_FLAG) $(DEBUG_FLAGS) -od$(RESULTS_DIR) -of$(RESULTS_DIR)$(DSEP)sanitize_json$(EXE) -i sanitize_json.d

11 changes: 7 additions & 4 deletions test/compilable/extra-files/json-postscript.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
#!/usr/bin/env bash

grep -v "\"file\" : " ${RESULTS_DIR}/compilable/json.out | grep -v "\"offset\" : " | grep -v "\"deco\" : " > ${RESULTS_DIR}/compilable/json.out.2
grep -v "\"file\" : " compilable/extra-files/json.out | grep -v "\"offset\" : " | grep -v "\"deco\" : " > ${RESULTS_DIR}/compilable/json.out.3
echo SANITIZING JSON...
${RESULTS_DIR}/sanitize_json ${RESULTS_DIR}/compilable/json.out > ${RESULTS_DIR}/compilable/json.out.sanitized
if [ $? -ne 0 ]; then
exit 1;
fi

diff --strip-trailing-cr ${RESULTS_DIR}/compilable/json.out.2 ${RESULTS_DIR}/compilable/json.out.3
diff --strip-trailing-cr compilable/extra-files/json.out ${RESULTS_DIR}/compilable/json.out.sanitized
if [ $? -ne 0 ]; then
exit 1;
fi

rm ${RESULTS_DIR}/compilable/json.out{.2,.3}
rm ${RESULTS_DIR}/compilable/json.out.sanitized
Loading