Add 'dub describe --data='

[Abscissa]

Expands on the recent dub describe --[string-]import-paths by adding a generalized dub describe --data=... which supports all fields of BuildSettings, not just importPaths and stringImportPaths. Multiple --data= flags can be used in one invocation of dub, and the results will be in the order requested on the cmd line.

This was discussed in #562. The "recursive vs non-recursive" distinction turned out to be both unnecessary and a bad approach, so that aspect was omitted here.

I'll add optional alternate output formatting (ie, the --format=(dmd|list), or possibly using --compiler=, described in #562) in a separate pull request. This PR includes the ability to automatically format for --data output for the default compiler, or the compiler specified by --compiler. This is the default behavior for --data.

If --data-list is used, then instead of formatting the output for a particular compiler, all data will be output line-by-line (just like --[string-]import-paths) and each set of --data= items will be separated by a blank line. Note that not all --data= fields are directly accepted by compilers (such as "pre-build-commands" or "working-directory"), so these fields are only allowed when using --data-list.

This pull replaces the defunct #566.

As discussed in #562, I'm considering reusing --compiler= instead of introducing --data-format=. The "list" mode could simply be enabled with a separate flag like --list-format. Actually, I'll likely do that within the next day or two. Now done.

[Abscissa]

Pinging, just to point out that automatically formatting for the requested (or default) compiler is now included in this PR.

[Abscissa]

Working example:

$ cd /home/nick/proj/dub

$ dub describe --data=main-source-file --data=options \
--data=versions --data=import-paths
'/home/nick/proj/dub/source/app.d' -debug -g -w -version=DubUseCurl -version=Have_dub '-I/home/nick/proj/dub/source/'

$ dub describe --compiler=ldc --data=options --data=versions
-d-debug -g -w -oq -od=.dub/obj -oq -od=.dub/obj -d-version=DubUseCurl -d-version=Have_dub

$ dub describe --data-list --data=options --data=versions
debugMode
debugInfo
warningsAsErrors

DubUseCurl
Have_dub

[Abscissa]

Example of CSV feature:

$ dub describe --data=options,versions
-debug -g -w -version=DubUseCurl -version=Have_dub

$ dub describe '--data=options, versions'
-debug -g -w -version=DubUseCurl -version=Have_dub

[s-ludwig]

Didn't make it yesterday due to sickness. Generally looks good AFAICT. These are the things that caught my eye:

• BuildSettings.libFiles: Currently all linker files that are passed through by the compiler are stored in sourceFiles and distinguished from source files using dub.compiler.compiler.isLinkerFile. It may or may not make sense to split this up into more fine-grained fields, but I'd keep that decision to a different PR and go with only sourceFiles for now. Instead of --data=lib-files, --data=linker-files might also be more useful. Or was this specifically meant as a way to keep the DUB link dependencies separate from other linker files?
• targetLookup/targetDescriptionLookup: To avoid the redundant storage, what about using size_t[string] instead and provide a ref inout(TargetDescription) lookupTarget(string) inout method?
• getBuildSettingBitField: Looks like that could be replaced by to!BuildSetting, or even __traits(getMember, BuildSetting, attributeName)
• buildPath(pack.path.toString(), ...) might misbehave on Windows for certain paths (not sure). I'd recommend to use (pack.path ~ ...).toNativeString() instead, or at least toNativeString instead of toString.

[s-ludwig]

I'll have a look at the failing Travis-CI build later, so that the refactor1 branch passes and this can be rebased.

[Abscissa]

BuildSettings.libFiles: Currently all linker files that are passed through by the compiler are stored in sourceFiles and distinguished from source files using dub.compiler.compiler.isLinkerFile. It may or may not make sense to split this up into more fine-grained fields, but I'd keep that decision to a different PR and go with only sourceFiles for now. Instead of --data=lib-files, --data=linker-files might also be more useful. Or was this specifically meant as a way to keep the DUB link dependencies separate from other linker files?

I agree, --data=linker-files sounds more useful, and more in line with my intent. Will fix.

targetLookup/targetDescriptionLookup: To avoid the redundant storage, what about using size_t[string] instead and provide a ref inout(TargetDescription) lookupTarget(string) inout method?

Sounds good. Will fix.

getBuildSettingBitField: Looks like that could be replaced by to!BuildSetting, or even __traits(getMember, BuildSetting, attributeName)

I would've thought that a to!BuildSetting would cause importers conflicts with std.conv.to (easily dealt with via local aliases tough). In any case, __traits(getMember sounds like a good idea, didn't think of that. Will fix.

buildPath(pack.path.toString(), ...) might misbehave on Windows for certain paths (not sure). I'd recommend to use (pack.path ~ ...).toNativeString() instead, or at least toNativeString instead of toString.

buildPath is pretty darn well-designed (although perhaps I should've used buildNormalizedPath.) From http://dlang.org/phobos/std_path.html:

Note that on Windows, both the backslash () and the slash (/) are in principle valid directory separators. This module treats them both on equal footing, but in cases where a new separator is added, a backslash will be used.

But sticking with existing dub convention is probably better regardless. Will fix.

[Abscissa]

I would've thought that a to!BuildSetting would cause importers conflicts with std.conv.to (easily dealt with via local aliases tough).

Nevermind, I see what you mean now.

BTW, __traits(getMember thinks that BuildSetting is an int, but to!BuildSetting works fine. Fixed now.

[Abscissa]

buildPath(pack.path.toString(), ...) might misbehave on Windows for certain paths (not sure). I'd recommend to use (pack.path ~ ...).toNativeString() instead, or at least toNativeString instead of toString.

buildPath is pretty darn well-designed (although perhaps I should've used buildNormalizedPath.) From http://dlang.org/phobos/std_path.html:

Note that on Windows, both the backslash () and the slash (/) are in principle valid directory separators. This module treats them both on equal footing, but in cases where a new separator is added, a backslash will be used.

But sticking with existing dub convention is probably better regardless. Will fix.

Actually, I still need to use build(Normalized)Path because I'm relying on it's ability to do this:

assert(buildPath("prefix", "/absolute/path") == "/absolute/path");

Ie, an absolute path automatically overrides any prefixes, which is very handy.

But I am changing buildPath to buildNormalizedPath just so the output is at least consistent with native (which is mainly important only for stylistic reasons, but good style is of course good).

Also, due to the buildNormalizedPath, using toNativeString instead of toString would just be redundant. Not that it really matters much either way, but I feel like toNativeString could lead anyone reading the code to assume it did matter.

[s-ludwig]

Okay, I'm just gonna trust you on the build(Normalized)Path issue. That behavior for buildPath(relative, absolute) is interesting. I wonder if it makes sense to change Path's operator ~ to do the same. The reason I chose to disallow it was because I was afraid of possible hidden and possibly security relevant bugs (at least in the vibe.d context).

[Abscissa]

The reason I chose to disallow it was because I was afraid of possible hidden and possibly security relevant bugs (at least in the vibe.d context).

Interesting point. Essentially this.

I don't think it matters in this PR because absolute paths need to be supported regardless.

More generally though, it's an interesting point. My gut feeling is that if you're trying to restrict directory paths, you really should be enforcing that at the "output" end anyway, after all path operations (ie, when you're actually using the resulting path) rather than purely relying on intermediate path operations to maintain the desired restrictions. But I guess that's a topic for a separate forum.

[Abscissa]

Currently all linker files that are passed through by the compiler are stored in sourceFiles

Unfortunately, BuildSettings.sourceFiles doesn't appear to actually contain the output lib of (non-sourceLibrary) library dependencies. Neither does PackageDescription.files.

Should I adjust TargetDescriptionGenerator to populate both BuildSettings.sourceFiles and PackageDescription.files with the getTargetFileName(...) for each dependency in linkDependencies?

[Abscissa]

Currently all linker files that are passed through by the compiler are stored in sourceFiles and distinguished from source files using dub.compiler.compiler.isLinkerFile. It may or may not make sense to split this up into more fine-grained fields, but I'd keep that decision to a different PR and go with only sourceFiles for now.

I just re-read this part. I think this is somewhat of an issue.

CLI users (ie, users of dub describe --data=...) don't have the benefit of running isLinkerFile(), at least without needlessly complicating things. So there's a need to be able to retrieve D files and static lib files separately. I could do a --data=linker-files and --data=d-files without adding extra fields to BuildSettings, but it would be awkward special casing, and I'm not sure I see any benefit to keeping linkerFiles and the corresponding dFiles out of BuildSettings. Although maybe I could implement BuildSettings.linkerFiles and BuildSettings.dFiles as functions instead, which may be better anyway.

[s-ludwig]

Another interesting thing may be the possible distinction between source files of different languages. My current plan is to support language selection in a per-package basis*, so "sourceFiles" and an additional "language" field would already be sufficient (as opposed to separate "dFiles", "cppFiles" etc.).

So my proposal is to generally split up the BuildSettings.sourceFiles field into sourceFiles and linkerFiles and eventually remove the isLinkerFile logic after a deprecation phase. The same would apply to the package description format ("Warning: Use linkerFiles instead of sourceFiles to ...").

Maybe it makes most sense at this point to already model the --data feature like that, while keeping the internal structures as they are now to avoid a possible impact on the existing functionality. So --data=source-files would simply get replaced by what --data=d-files currently does and --data=d-files would get dropped.

* Any arguments for or against this approach or other proposals would be very welcome at this point. The other obvious alternative would be to offer separate "dFiles"/"cppFiles"/... fields, but that would not be compatible with most IDE project representations and would complicate the implementation of any "generator", as well as any tool that currently uses the "dub describe" output.

[Abscissa]

I completely agree with moving the linker files out of sourceFiles and into a linkerFiles. And also with making --data= lead the charge on that change.

I don't have a problem with sourceFiles containing sources of all languages, however I think that on the command-line at least, it may prove problematic (or at least awkward) to not be able to request sources for a specific language. But, I'm fine with taking a wait-and-see approach on that for now if you'd prefer.

[s-ludwig]

I don't have a problem with sourceFiles containing sources of all languages, however I think that on the command-line at least, it may prove problematic (or at least awkward) to not be able to request sources for a specific language. But, I'm fine with taking a wait-and-see approach on that for now if you'd prefer.

With the one language per package approach, also each "target" would be comprised of files of single programming language, so with the current approach to list only the build settings of the root target, it would still result in a uniform list of source files (the external script/tool would just have to test the output of dub describe --data=language or similar).

A propos targets - we'll also need a --data=targets switch, as well as a --target=<targetname> one to select build settings of a target different than the root one. At least at some point when non-rdmd builds are to be fully supported.

[Abscissa]

With the one language per package approach, also each "target" would be comprised of files of single programming language, so with the current approach to list only the build settings of the root target, it would still result in a uniform list of source files

Right. That's pretty much why I'm fine leaving it up as as "meh, we can wait and see if it even ends up needed at all" instead of actually doing a d-files right now.

As per your suggestions, I'll remove the d-files I added a couple commits ago, and modify --data=source-files to omit any linker files that may be in sourceFiles.

(the external script/tool would just have to test the output of dub describe --data=language or similar).

I'm not sure I understand your --data=language suggestion. Do you mean dub would auto-detect the predominant language in a package and --data=language would output, for example, "d" or "cpp"? How would dub detect the language? Just count the number of sources files for each known file extension?

A propos targets - we'll also need a --data=targets switch, as well as a --target= one to select build settings of a target different than the root one. At least at some point when non-rdmd builds are to be fully supported.

By "targets" do you mean a package's dependencies? What would be the need of dub describe foo --target=bar --data=... as opposed to just doing dub describe bar --data=...? Note that the latter already works in this PR (I didn't even have to do anything to support it! :) )

BTW, speaking of future enhancements: After this all gets merged into master, I'd like to expand on the changes in my #593 PR by utilizing all this work for --data= to automatically include all this --data information in the environment variables passed to the custom build commands.

[s-ludwig]

I'm not sure I understand your --data=language suggestion. Do you mean dub would auto-detect the predominant language in a package and --data=language would output, for example, "d" or "cpp"? How would dub detect the language? Just count the number of sources files for each known file extension?

No my idea was to have a "language" field in the package description that just defaults to "D". So you'd always have to specify the language and DUB would just stupidly pass all source files to the corresponding compiler, regardless of their extension.

By "targets" do you mean a package's dependencies? What would be the need of dub describe foo --target=bar --data=... as opposed to just doing dub describe bar --data=...? Note that the latter already works in this PR (I didn't even have to do anything to support it! :) )

Target means a binary target (.lib/.exe/.so/..). Usually a target corresponds to a package, but multiple packages may be combined into a single target when --combined or "targetType": "sourceLibrary" is involved. So often dub describe <target> would be very similar to dub describe --target=<target> in that regard, but not always. Also, some settings, such as build requirements or version identifiers are inherited from the root project upwards the dependency tree; dub describe <target> also wouldn't catch those.

BTW, speaking of future enhancements: After this all gets merged into master, I'd like to expand on the changes in my #593 PR by utilizing all this work for --data= to automatically include all this --data information in the environment variables passed to the custom build commands.

Sounds good. This is one of the things I always wanted to do but never got around to.

[Abscissa]

As per your suggestions, I'll remove the d-files I added a couple commits ago, and modify --data=source-files to omit any linker files that may be in sourceFiles.

Ok, done. Ready for another round of reviewing.

[s-ludwig]

I would just have left BuildSettings alone for now, because a lot of other code uses it, but isn't yet adjusted to handle the linkerFiles field. But, well, let's just keep it and get this done. I'll open a ticket and put this on the list for the next release.

[Abscissa]

Shouldn't target.buildSettings.linkerFiles first be populated with the reversely filtered source file list?

Not sure what you mean, can you clarify?

I would just have left BuildSettings alone for now, because a lot of other code uses it, but isn't yet adjusted to handle the linkerFiles field.

Note that BuildSetting.sourceFiles still includes the linker files (even though --data=source-files is adjusted to exclude them.) So I don't think this should affect any other code since, for other code, there's just simply another field now available.

Supporting --data=linker-files without BuildSetting.linkerFiles existing would require some awkward special-casing.

[s-ludwig]

Shouldn't target.buildSettings.linkerFiles first be populated with the reversely filtered source file list?
Not sure what you mean, can you clarify?

I mean the sourceFiles array gets cleared of all linker files, but the linkerFiles array doesn't get those added, so they just get lost.

Note that BuildSetting.sourceFiles still includes the linker files (even though --data=source-files is adjusted to exclude them.) So I don't think this should affect any other code since, for other code, there's just simply another field now available.

Yeah right, it's just that this has the risk of introducing code that fails in various places (i.e. someone adds code that adds to linkerFiles without realizing that it actually isn't used anywhere other than --data=). But if everything gets adjusted soon anyway, I don't see this being an issue in practice.

[Abscissa]

I mean the sourceFiles array gets cleared of all linker files, but the linkerFiles array doesn't get those added, so they just get lost.

They get added right here, in TargetDescriptionGenerator.generateTargets, same place as where they get added to sourceFiles:
https://github.com/D-Programming-Language/dub/pull/572/files#diff-7cf5b4dec822eb38161e4610594e395fR56

Yeah right, it's just that this has the risk of introducing code that fails in various places (i.e. someone adds code that adds to linkerFiles without realizing that it actually isn't used anywhere other than --data=). But if everything gets adjusted soon anyway, I don't see this being an issue in practice.

Ahh, I see. I can prep a PR for that once this one gets merged.

[s-ludwig]

They get added right here, in TargetDescriptionGenerator.generateTargets, same place as where they get added to sourceFiles:

I mean linker files that are defined directly within the package description as source files (e.g. "sourceFiles": ["./some.lib", "app.def", "src/main.d"]). Before the linker files are filtered out of the sourceFiles array, those should simply first be added to linkerFiles. In the TargetDescriptionGenerator I'd just write the link dependencies to linkerFiles (keeping the old behavior for sourceFiles).

[Abscissa]

I mean linker files that are defined directly within the package description as source files (e.g. "sourceFiles": ["./some.lib", "app.def", "src/main.d"]). Before the linker files are filtered out of the sourceFiles array, those should simply first be added to linkerFiles.

Oh, ok I see. I wasn't aware of that possibility as I haven't needed to do that yet. Now fixed.

In the TargetDescriptionGenerator I'd just write the link dependencies to linkerFiles (keeping the old behavior for sourceFiles).

Fixed.

[s-ludwig]

LGTM

[s-ludwig]

I'll open a PR for the refactor1 branch now.