Make --rdmd-default-compiler optional

[marler8997]

No description provided.

[dlang-bot]

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.

[CyberShadow]

As previously discussed, let's not do this.

[marler8997]

This change is different than the previous. It makes the option optional instead of removing it.

[WebDrake]

Leaving aside pros and cons of this particular change for the moment, I note that many of the recent changes suggested seem to have been with the aim of making it simpler (for a certain definition of "simpler"...) to run rdmd_test directly.

I'd like to understand why that's a desired outcome, because a large part of the work done on revising the design was intended to make it so that developers didn't really need to care about rdmd_test itself: instead everyone can just run make -f {posix.mak,win32.mak} test_rdmd.

Is there a use-case that I didn't anticipate, that makes it useful or necessary to run rdmd_test directly?

[marler8997]

Being able to run rdmd_test directly allows one to change the parameters without having to change the Makefile. This is useful if you need to run different variations back to back (which I have been doing). If I only used the Makefile, I would have to open/edit/save the makefile for every variation I want to run.

[WebDrake]

Which options do you need to set? And just for clarity -- are you working on POSIX or on Windows?

[marler8997]

I work and test on both POSIX and Windows. So far I execute multiple back to back runs of rdmd_test where I var the compilers option, the rdmd binary argument, the model option and the verbose option.

[WebDrake]

Right. But you shouldn't need to run rdmd_test directly for any of that: where you need them to vary, you can just set the RDMD_TEST_EXECUTABLE, RDMD_TEST_COMPILERS, and MODEL option.

i.e.

make -f {posix,win32}.mak test_rdmd RDMD_TEST_EXECUTABLE=... RDMD_TEST_COMPILERS=... MODEL=...

Is that problematic for you in some way (e.g. too verbose)?

[WebDrake]

OK, I looked into this a bit deeper: it looks like the MODEL option at least is tricky (for posix.mak) because that clashes with other aspects of the makefile (e.g. how the default DMD is selected). So that at least needs to be customizable separately from the makefile. Looking at --help output we're also missing a means to set the --concurrency flag from the makefile.

I think everything else should be covered, though.

[marler8997]

As far as I know, Digital Mars make doesn't allow you to override variables on the command line which means your suggestions don't work on Windows. Also you didn't mention the verbose flag. RDMD_TEST_VERBOSE?

In any case, I don't think that having a Makefile interface means we should forget about the program's command line interface. You can tell developers to use the Makefile interface, but they still need to understand rdmd_test's interface when they look at the code. It's just now they have an extra step of translating the Makefile interface to the rdmd_test command line interface. Furthermore, if you tell developers they should always just use the Makefile interface, you're adding unnecessary coupling between the Makefile and the command line program because now you have to support ALL of rdmd_test's command line options in the Makefile (like MODEL and VERBOSE and whatever else we may want to add). I think the better option would have been to have the Makefile just use the default behavior and not add an extra interface that wraps rdmd_test. Just defer developers to execute rdmd_test directly if they want non-default behavior.

[WebDrake]

Also you didn't mention the verbose flag. RDMD_TEST_VERBOSE?

VERBOSE_RDMD_TEST=1. But TBH I think we could simplify that and just make rdmd_test always output verbosely. Or invert things so that there's a -q|--quiet option. My experience is that it's good to always see the detailed test output. What do you think?

As far as I know, Digital Mars make doesn't allow you to override variables on the command line which means your suggestions don't work on Windows.

That's what I was worried about :-(

Anyway, this makes things clearer. For now, we can't avoid scenarios (at least on Windows) where people need to run rdmd_test directly, and I apologize for not appreciating that better when I introduced these changes. However, we need to distinguish between making it easier to run directly (good!), versus making the tests less rigorous (bad!).

So, with respect to this specific PR, the intent is good (making it easy to run rdmd_test without having to specify any flags), but it has an unfortunate impact: it means that by default, the test suite is less rigorous, because by default, it won't validate that rdmd behaves as its builder intended.

That's really the opposite of how a test suite ought to behave: by default, it ought to be as rigorous as possible, with the possibility to request only a subset of the tests as an optional extra. (Think how e.g. make test should by default carry out all the tests, whereas a more focused target can be invoked if one wants to e.g. only test one application.)

So, by that standard, unfortunately, this PR still won't wash, despite the improvements made since the first version of these changes. Ease of use shouldn't trump the requirement to give the test suite all the input it needs to carry out a rigorous test. That's even before we get to the question of whether it is reasonable to try to extract test settings from rdmd's own help text (which is a quite imaginative way to do things, but ultimately means that the test parameters are being set by the thing being tested, which is rather dodgy).

However, I will take a look at the rdmd_test UI and see what I can do to further simplify things, now that it's clear that we need to support running rdmd_test directly.

Longer term, it would be good to discuss the sort of test cases you're running that require these manual invocations, because the fact that you have to do this suggests that test coverage might not be as robust as it ought to be. Maybe we can find a way to extend the range of stuff that is available via a simple make test.

Lastly, but importantly:

rdmd_test doesn't seem to be working on windows anymore.

Can you describe the errors that you're encountering? I don't have a Windows build setup in place right now, so can't test directly. :-(

[marler8997]

I agree with your points. In order to prevent someone from accidentally removing the --rdmd-default-compiler option, I added a WARNING message when this occurs. This makes it easier to call rdmd_test directly while putting in a measure to keep the test from removing option that it shouldn't.

[marler8997]

I think my particular windows box is failing because my compiler is a dmd.bat file that performs setup and redirects to another compiler. They way rdmd_test currently works, it needs to be a .exe.

This would be fixed by using spawnShell instead of execute, but this has always been broken so not related to any of your recent changes.

[WebDrake]

I agree with your points. In order to prevent someone from accidentally removing the --rdmd-default-compiler option, I added a WARNING message when this occurs. This makes it easier to call rdmd_test directly while putting in a measure to keep the test from removing option that it shouldn't.

That was a nice touch. But it kind of emphasizes the point -- the default behaviour of rdmd_test (i.e. the behaviour when running with any optional flags omitted) should not be something that results in a warning.

[marler8997]

the default behaviour of rdmd_test (i.e. the behaviour when running with any optional flags omitted) should not be something that results in a warning.

What should it result in?

[joseph-wakeling-sociomantic]

Sorry for taking a while to get back to you on this.

What should it result in?

The default behaviour should be to apply all test cases, rigorously. And that means that when invoking it, we need to provide all required information to allow those tests to be rigorous.

When I was beginning the rewrite of rdmd_test, my first draft just intended to drop the --build-compiler flag. Then I realized: oh, no, that's used by the fallback test... so I thought, maybe I should just leave it as-is, defaulting to "dmd" and allowing the user to override that with the flag only if they wanted to.

And then, having thought it through, I realized: that could easily lead to false positives. For example, if I introduce a bug into how defaultCompiler is defined inside rdmd.d, e.g. that it's "dmd" whatever the compiler used to build rdmd ... then running rdmd_test with a default setting for the expected fallback compiler, could easily miss that bug.

In other words, as long as rdmd's default compiler is intended to be determined by the compiler used to build it ... then validating correct behaviour unavoidably involves telling the test suite what that compiler is.

This PR introduces the same chance for false-positive test passes.

Probably the long-term best way to address this is to update rdmd to support a config file, so that things like the choice of default compiler are no longer hardcoded by the build. Then it would be easy to rework the test suite to set up a test config with a sensible choice of settings.

[marler8997]

Yes you have a valid point. However, the warning message that occurs when you omit this argument seems fine to me. The tradeoff is that now you don't have to type out that long command line option every time for the simple check that the "defaultCompiler" matches what you expect.

[joseph-wakeling-sociomantic]

If a warning is output, then it's easy for someone to tweak CI to remove the --rdmd-default-compiler flag (whether deliberately or unintentionally), and if no one is being virtuous and reading the build logs in detail, it'll be completely missed, because it results in no error.

I do sympathize over the extra typing, but I wouldn't prioritize it over ensuring that the test suite is rigorous by default.

[marler8997]

If a warning is output, then it's easy for someone to tweak CI to remove the --rdmd-default-compiler flag (whether deliberately or unintentionally), and if no one is being virtuous and reading the build logs in detail, it'll be completely missed, because it results in no error.

I understand this concern but this isn't really a strong argument. If someone can remove this flag, they can just as easily remove the whole command. You're locking a door to a house that has no walls. Locking the door might make you "feel" more safe but that's all it is...the appearance of safety. This isn't a big deal though unless this is impeding other features...such as the one I've implemented in this PR.

[joseph-wakeling-sociomantic]

If someone can remove this flag, they can just as easily remove the whole command.

I presume a slightly higher level of scrutiny to removing a test-case from the test suite, than making a small and easy-to-miss-the-significance tweak to how CI invokes the test suite.

Bottom line, I think there is consensus on the review side that we should not make this change to rdmd_test. Shall we close here and move forward?

FWIW I would happily accept a patch to rename --rdmd-default-compiler to just --default-compiler. Not great, but at least it saves you some keystrokes. If you'd like that I can write and submit it myself.

We could also implement a single-character variant of that flag to the same purpose.

[marler8997]

I don't see consensus yet. @CyberShadow's single comment assumed this change was the same as the previous PR.

[joseph-wakeling-sociomantic]

Fair enough; @CyberShadow have you changed your mind?

FWIW I think the fundamental dodginess of parsing the app's help output to determine the input to a test case, is reason enough to reject this PR.

[joseph-wakeling-sociomantic]

And bear in mind -- if we implement a friendly single-char flag for the compiler, your burden is going to be limited to:

./rdmd_test -D dmd path/to/rdmd

... which doesn't seem onerous.

[marler8997]

FWIW I think the fundamental dodginess of parsing the app's help output to determine the input to a test case, is reason enough to reject this PR.

This was a valid argument with the original PR, but not this one.

[joseph-wakeling-sociomantic]

This was a valid argument with the original PR, but not this one.

Oh come on, that won't wash. This PR means that, by default (warning or no), rdmd_test will determine the input of the fallback test -- i.e. the choice of the default compiler name -- by parsing the tested rdmd's help output.

And warning or no, default or no, it shouldn't be doing that at all.

[marler8997]

rdmd_test will determine the input of the fallback test

There are 2 things being tested here

1. Compiler names match (name from "help text", name from --rdmd-default-compiler and the name of the compiler that rdmd "falls back" to)
2. The rdmd fallback mechanism

If --rdmd-default-compiler is not given, the fallback logic and the "help text"/"fallback" names matching is still verified. It's just that the tester is choosing not to enforce that the name to be something in particular.

Saying that using the "help text" to verify the "fallback" name isn't dodgy...it's already being done since the "help text" name must already match the --rdmd-default-compiler option. This is again, a weak argument.

[WebDrake]

If --rdmd-default-compiler is not given, the fallback logic and the "help text"/"fallback" names matching is still verified. It's just that the tester is choosing not to enforce that the name to be something in particular.

And that's one of the problems.

Saying that using the "help text" to verify the "fallback" name isn't dodgy...it's already being done since the "help text" name must already match the --rdmd-default-compiler option. This is again, a weak argument.

There is a fundamental difference between

• testing that an output of the thing being tested matches some predetermined expectation

• testing that one output of the thing being tested matches a different output of the thing being tested

Self-consistency is nice, but things can be self-consistent and still wrong.

[marler8997]

Self-consistency is nice, but things can be self-consistent and still wrong.

I'm not arguing against this...I'm saying your "dodgy" comment wasn't a good argument. The legitimate argument you've given is "this PR makes compiler name verification optional instead of required". I think the time it saves typing out the extra argument is worth allowing it to be optional, especially since it prints a warning when you don't provide it. Is the PR justified? Maybe, I think it's a matter of opinion. Since I have to run this all the time I prefer to make my life easier, so you know what my opinion is.

[WebDrake]

Since I have to run this all the time I prefer to make my life easier, so you know what my opinion is.

I do recognize that you have a usability issue here. But I think we should value robustness of the test suite over the difficulty of typing a few extra characters for each manual invocation.

Perhaps we should discuss in more detail the variety of ways in which you need to run the script. It might be that we could find a way to provide greater test coverage just from a single invocation. It would be good to see what we can do in that respect, before considering weakening the robustness of the tests.

[marler8997]

I do recognize that you have a usability issue here. But I think we should value robustnes...

This PR is not "sacrificing robustness". It is making one aspect of the test "optional".

[WebDrake]

It is making one aspect of the test "optional".

If one part of the test is optional rather than obligatory, then by definition the test suite is less robust.

Come on, let's not go round in circles. We have our positions on this. The only constructive way to go forward in this discussion is to discuss the details of your usage so that we can examine whether we can find a way to simplify that for you.

[marler8997]

If one part of the test is optional rather than obligatory, then by definition the test suite is less robust.

It's not optional when running the test via the Makefile, only optional for those running the test directly. And saying that this makes the "test suite" less robust is misleading...it just makes a part of the test optional. If you have a test suite with multiple parts that can be run independently, that doesn't make it "less robust" just because you can run one without the other. This is clearly a logical fallacy.

Come on, let's not go round in circles. We have our positions on this.

At this point I'm just corrrecting the false claims you are making. If you don't want me to correct you then don't make them. I've already summarized the case...but you continue to add invalid points to it so I feel obliged to correct them.

[WebDrake]

If you have a test suite with multiple parts that can be run independently, that doesn't make it "less robust" just because you can run one without the other. This is clearly a logical fallacy.

There's no problem with test-suite options that allow the user to explicitly request, "Please run only a subset of the test suite."

There is a problem when the default behaviour of the test suite is to run only a subset of its tests. Which is clearly the case here.

[marler8997]

The default is to use the Makefile...so no, that's not the case here.

[WebDrake]

The default collection of tests should be the same whether invoked via the makefile or whether by running rdmd_test directly. That's by design.

[marler8997]

The default collection of tests should be the same whether invoked via the makefile or whether by running rdmd_test directly. That's by design.

This statement is demonstrably false. The tests would only be the same if the developer mimicked the same command line arguments as the makefile, i.e.

make -f posix.mak test_rdmd

rdmd rdmd_test.e

These 2 are certainly not the same thing. You would have to include all of the command line parameters for it to be the same (regardless of whether or not this PR is integrated)

rdmd rdmd_test.d rdmd --test-compilers=$(DMD) --rdmd-default-compiler=$(DMD)

Please refrain from adding more false claims to this conversation, it's exhausting having to constantly correct them. You have brought up a valid point, namely, that this PR makes compiler name verification optional instead of required when running rdmd_test directly. It's not a strong argument, but it's a legitimate one. Everything else you have brought up are not legitimate arguments.

[WebDrake]

This statement is demonstrably false. The tests would only be the same if the developer mimicked the same command line arguments as the makefile

The collection of tests is the same. The particular configuration parameters are different.

That is fundamentally different from running rdmd_test directly excluding one test case by default.

[marler8997]

The collection of tests is the same. The particular configuration parameters are different.

Demonstrably false again

rdmd_test --compilers=../dmd1,../dmd2

Not the same as

make -f posix.mak test_rdmd

The command line parameters need to be the same for both to be the same.

[WebDrake]

rdmd_test --compilers=../dmd1,../dmd2

... where the same set of tests get repeated, once with one compiler, the second time with another.

[marler8997]

... where the same set of tests get repeated, once with one compiler, the second time with another.

Yup, you see why your statement was false now?

[WebDrake]

No. I see that you are failing to see the distinction between test cases, versus test configuration.

The bottom line of this is that you are asking that, by default, running rdmd_test should exclude a test case that I would like to always be performed. And you're asking this so that you can avoid typing a few more characters on the command line.

That is not, I think, a recipe for good or reliable testing.

Now, I've offered to discuss with you your use-cases to see if we can avoid the need for you to perform so many individual runs of rdmd_test. You haven't chosen to engage with that offer.

So, with that in mind, I suggest that we stop this discussion now and allow @CyberShadow to respond on whether he's changed his mind about this being a worthwhile change to the test suite or not.

[marler8997]

My current plan is to wait for this PR to get merged until I continue work on rdmd. If it doesn't get merged, I'll continue to use my rdmd replacement (https://github.com/marler8997/rund) and not worry about upstreaming those changes into rdmd.

If I have to keep dealing with this kind of resistance for such simple changes, it's really not worth my time.

[CyberShadow]

Hi,

Sorry to see disagreements and extended debates about such minor things again.

@marler8997 The time spent debating this change could have been used hundred-fold to write a small wrapper script around rdmd_test that fits your workflow.

@WebDrake / @joseph-wakeling-sociomantic Consider whether debating this has been the best use of your time.

There will inevitably be disagreements (boiling down to subjective differences of opinion) among contributors in any sufficiently-large project. Ultimately we want to maximize the utility function of our time (how effectively spending it advances our ultimate goals), whether that's contributing to a software project we like or otherwise improving our life and the world around us. Please consider how the time spent on this issue ranks on your utility functions. Does the negative impact of the worst-case scenario (the command line syntax of the rdmd test suite changed in a way that doesn't align with your vision) outweigh the cost of this debate?

I am going to close this PR. Not because I agree with one side or the other - both sides have presented good arguments - but due to the disproportional difference between the importance of this change and decision, and the time needed to resolve it. As a matter of policy I would like to do the same with similar future PRs and debates, because it really is not a good use of anyone's resources.

Thank you for your consideration.

[marler8997]

Any work that I've done with rdmd where @WebDrake has been involved has been met with nothing but relentlessness debate and resistance. Up to this point my changes were merged anyway. It seems that now I've got little choice but to give up on making changes to rdmd. I was planning on finishing (https://github.com/dlang/tools/pull/292/files) but will instead focus my time in other areas. I will close that PR as well, if someone else wants to take it over they are free to do so.

[CyberShadow]

Up to this point my changes were merged anyway.

I don't regret merging the good contributions, and thanks for those. I do kind of regret merging #335, because what I said above applied to it as well.

I was planning on finishing (https://github.com/dlang/tools/pull/292/files) but will instead focus my time in other areas. I will close that PR as well, if someone else wants to take it over they are free to do so.

Unlike this PR and #335, #292 is actually meaningful and worth spending time on, but it is your decision to make. I hope you'll understand that we can't afford to be held hostage either.

[marler8997]

I don't regret merging the good contributions, and thanks for those. I do kind of regret merging #335, because what I said above applied to it as well.

Well feel free to revert it, I won't be spending my time with rdmd_test anymore so it's no skin off my nose.

Unlike this PR and #335, #292 is actually meaningful and worth spending time on, but it is your decision to make. I hope you'll understand that we can't afford to be held hostage either.

Yeah I understand. It just comes down to people, I've tried to work with @WebDrake but its just not working, everyone's time will be better spent if I work on other things. Good luck, I hope someone will pick up #292 for D's sake...rdmd's performance is really bad so it's definitely worthwhile.