dlang
WIP: Forward flag -color to compiler calls
Need help determining how to forward options.colors_mode to setting the instance of BuildOption.color which should, afaict, be forwarded automatically to DMDCompiler.s_options and LDCCompiler.s_options.
Update: I think I nailed it.
Can you have a look, @Geod24?
FYI, @kinke.
should we only pass explicit color argument to dmd or also implicit as set by NO_COLOR env (or other external ways)?
What's important to me is that the color switch (-fdiagnostics-color=always for GDC btw) isn't used for the build hash - enforcing colors should not lead to cached packages being rebuilt. Otherwise colors could already be enforced via DFLAGS env var.
Should BuildOption.color be part of inheritedBuildOptions in buildsettings.d?
What's important to me is that the color switch (-fdiagnostics-color=always for GDC btw) isn't used for the build hash - enforcing colors should not lead to cached packages being rebuilt.
Can anybody point me to the source location where this hash is calculated? Searching now...
Update: Is it computeBuildID(). If so, the parameter buildSettings.options is not included in the hash and buildsettings.dflags doesn't contain the compiler flag when --colors=on is passed to dub.
Update: Wrong, I need to adjust the expression
(cast(uint)buildsettings.options).to!string
in computeBuildID. On to it...
Update: Resolved via f53d7c05, @kinke.
should we only pass explicit color argument to dmd or also implicit as set by NO_COLOR env (or other external ways)?
I don't know.
Otherwise colors could already be enforced via DFLAGS env var.
Are you saying we shall exclude color flags from DFLAGS env var aswell, @kinke?
[Nope, just saying those rebuilds prevented me from working around this via DFLAGS for now.]
[Nope, just saying those rebuilds prevented me from working around this via DFLAGS for now.]
So is this good to go, @thewilsonator @Geod24?
I'm on vacation this week so on mobile most of the time but I'll try to take a look one evening.
Changes look good but please add a test.
Is it ok if I extend the existing tests for the --color(s) flag?
Changes look good but please add a test.
Do you have a suggestion for how to add a test for this? I vote for grepping for the forwarded compiler flag in the output of a call to dub -v ... for both dmd, ldc and gdc? This prevents use from having to build with all 3 compilers.
Ping, @veelo.
Changes look good but please add a test.
Do you have a suggestion for how to add a test for this? I vote for grepping for the forwarded compiler flag in the output of a call to
dub -v ...for both dmd, ldc and gdc? This prevents use from having to build with all 3 compilers.
I think that will be good enough. If you really would want to check the existence of color in the compiler output you could probably check for the presence of ANSI escape codes, but that requires you to know how each compiler formats its output, which can change between releases.
I'm trying to run the test colored-output.sh as
DC=/usr/bin/dmd DUB=../bin/dub CURR_DIR=. ./colored-output.sh
under test sub-dir but that fails
as
DC=/usr/bin/dmd DUB=../bin/dub CURR_DIR=. ./colored-output.sh
0
[ERROR] ./colored-output.sh:8 command failed
. What am I doing wrong?
Need help. Ping, @Geod24 @thewilsonator @veelo.
IMO, dub desperately needs a obvious and trivial way to run individual and all tests along with a documentation for it.
Just to rule out the obvious: did you try to run the command at line 8 by hand (after cd 1-exec-simple)?
Just to rule out the obvious: did you try to run the command at line 8 by hand (after
cd 1-exec-simple)?
That's not the command that fails.
I found it out now the problem was that the piping hid the real error message. On to it now again.
Figured it out. The project build and test command I currently use is
dub build && cd test && DC=dmd DUB=~/Work/dub/bin/dub CURR_DIR=. ./colored-output.sh
. What are yours? Is this documented somewhere in dub? If not we need to add it. Along with a convenience wrapper script that is present in the root directory and that is obvious how one uses.
Questions. Why isn't variable
CURR_DIRdefaulted to${BASH_SOURCE[0]}?DCdefaulted todmd?DUBdefaulted to../bin/dub?
Going green now for the dmd forwarding case. I'm gonna see if can make the ldc and gdc cases work aswell.
IMNSHO all shell scripts should be eliminated from all repositories. D is great for scripting, and cross platform.
Questions. Why isn't variable
CURR_DIRdefaulted to${BASH_SOURCE[0]}?DCdefaulted todmd?DUBdefaulted to../bin/dub?
2 and 3, because that would make it way too easy to accidentally make things work when we fail to pass the right compiler. 1 I don't remember but I think there's a reason. In any case, this test suite infrastructure was started almost 10 years ago by a college student that had no idea what he was doing at the time, so yeah we should rewrite it in D. I'm on a (side) quest of moving environment reads / IO out of some deeply nested functions so that we can actually unittest things (much faster than integration tests).
@nordlow : Whenever you want to update a PR, please do git fetch upstream && git rebase upstream/master, it makes life much easier.
How long does buildkite/dub normally take? It's been running for 2 hours now.
Hmm, I might have forgotten to forward coloring to compilers in the case where --color=automatic...
Hum, probably :)
Shall we do anything about this?
To be strict about this we need to add to change the type of the flag BuildOption.color from bool to enum color { automatic, on, off } which will require significant refactor of the type of build options as it currently assumes all build options boolean. Soon or later we need to deal with this. Shall we bother with this now?
Don't the compilers already display colors automatically (i.e., if output is a TTY)? In that case not forcing anything will already do the right thing in the default case.
change the type of the flag BuildOption.color from bool to enum color { automatic, on, off } which will require significant refactor of the type of build options as it currently assumes all build options boolean.
Which leads me to question how this is currently implemented.
color = 1<<26, /// Colorize output (-color)
This adds a flag to force color on, no? And we don't have a flag to force color off. Thus from the tristate we only forward one state to the compiler currently.
I don't know if this is needed in any practical situation, but you could add another flag nocolor to force color off. Then if both flags are false it would imply automatic. Both flags true would be illegal.
I don't know if this is needed in any practical situation, but you could add another flag
nocolorto force color off. Then if both flags are false it would imply automatic. Both flags true would be illegal.
We could add that but that is bad design because it introduces error states. I vote for refactoring build options to be able to store flags other boolean such as, in this case, enum color.