Use multiple different test compilers when invoking rdmd_test from Travis CI #297

WebDrake · 2018-01-23T23:12:12Z

These patches introduce a simple proof-of-concept for using rdmd_test's new --test-compilers flag in CI. The posix.mak makefile has been extended with a new RDMD_TEST_COMPILERS variable, which can be used to set the choice of compilers to be tested with rdmd_test. This is then used to update Travis CI settings to test with both dmd and ldmd2 (with the latter being installed as a distro package). gdmd is not included for the time being since installing it would have been trickier (there is a gdc distro package available but it does not include the gdmd perl script).

The last patch is added just for testing purposes and should probably be dropped before this PR is accepted: it makes the test_rdmd make target run rdmd_test verbosely so that we can clearly see the multiple compilers in action.

This should probably not be the final form for setting up multi-compiler tests of rdmd, but it serves as an initial stage which should at a minimum flag up issues like that observed with #271.

No changes have been made to windows makefiles, since this is not required by Travis CI. I would anticipate this being fairy easy to add if desired, but it might be a good idea if this was done by someone with more Windows experience in a separate PR.

dlang-bot · 2018-01-23T23:12:13Z

Thanks for your pull request, @WebDrake! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

WebDrake · 2018-01-23T23:14:42Z

Hmmm, looks like the ldc package is only available for 64-bit systems, and hence the 32-bit Travis tests are failing without even getting to rdmd_test. I'll look into how best to address this tomorrow.

WebDrake · 2018-01-23T23:28:38Z

Ho hum, failing on all counts. Does anyone more familiar with the Travis setup than me have any idea why ldmd2 is not being found for the 64-bit test case?

wilzbach

You need to fetch the LDC archive via the install script (built-in feature of Travis). Don't use the Ubuntu/Debian packages - they are outdated and not officially maintained.

wilzbach · 2018-01-23T23:43:11Z

.travis.yml

@@ -11,6 +11,7 @@ matrix:
        apt:
          packages:
            - g++-multilib
+            - ldc


Use d: ldc - the install script will take care of the rest

To avoid any confusion you want something like this:

language: d d: - dmd-nightly - dmd - ldc

wilzbach · 2018-01-23T23:46:01Z

FYI: the LDC release archive as fetched by d: ldc supports 32-bit.

WebDrake · 2018-01-24T10:26:18Z

You need to fetch the LDC archive via the install script (built-in feature of Travis).

Cheers, will do. Does this also support install of GDC (including or excluding the gdmd perl script)?

Don't use the Ubuntu/Debian packages - they are outdated and not officially maintained.

Well, that's kind of the point. If we want to guard against breaking changes in rdmd then it needs to be measured against the D compilers that are actually out there and available in distributions. We don't want a new user installing LDC and GDC from their distro archive, then installing DMD from the dlang.org package, and finding that rdmd breaks with their other compilers.

Saying, "The latest versions of all compilers have feature X, and so rdmd can use it" is still a breaking change, and nice to avoid as much as possible. That doesn't have to mean forgoing new features where they are available, just that it's good if we use them in ways that have graceful fallbacks.

Anyway, I'll follow up with a fixup patch, and assuming that works, we can squash and finalize review.

WebDrake · 2018-01-24T10:29:24Z

Fixup patch pushed. If this works, I'l squash, and we can finalize review.

WebDrake · 2018-01-24T10:32:11Z

Ah. This appears to be doing what I was afraid it might: the d: settings interfere with the Travis test matrix. We don't want LDC to be an extra compiler test case; we want LDC to be available within the existing test builds (32- and 64-bit DMD).

@wilzbach can you advise?

wilzbach · 2018-01-24T10:57:44Z

Cheers, will do. Does this also support install of GDC (including or excluding the gdmd perl script)?

Yes, including gdmd.
FYI: https://dlang.org/install.html

Saying, "The latest versions of all compilers have feature X, and so rdmd can use it" is still a breaking change, and nice to avoid as much as possible. That doesn't have to mean forgoing new features where they are available, just that it's good if we use them in ways that have graceful fallbacks.

You can select specific versions e.g. ldc-1.2.0
I just skimmed over your last response and thought you had troubles getting ldc setup correctly.
If you want to use ldc from Ubuntu/Debian, I would recommend using CircleCi, because

it allows to use Docker images + gives sudo access -> pick your favorite distribution + no problem with old sources
we already build CircleCi docker images based on ubuntu/17.10 (see e.g. Allow to run the DMD testsuite on hardened systems dmd#7420)
it allows ssh access -> it's really easy to debug failures

It's not enabled here, but it would take two clicks to do so. The disadvantage of CircleCi is that it's harder to spawn matrix builds, so Travis might be a good choice here too.

Ah. This appears to be doing what I was afraid it might: the d: settings interfere with the Travis test matrix. We don't want LDC to be an extra compiler test case; we want LDC to be available within the existing test builds (32- and 64-bit DMD).

I assume you want this?

d:
- dmd
- ldc
- gdc
os:
- linux
- osx
env:
- MODEL=64
- MODEL=32

Travis will generate a Cartesian product out of these.
You can use OSX on Travis as

we stopped using Travis for dlang/dmd
The Travis people kindle increased our limit of 2 free OSX jobs to 5 concurrent OSX jobs

wilzbach · 2018-01-24T11:18:38Z

travis.sh

@@ -30,6 +30,7 @@ install_digger
 dmd --version
 rdmd --help | head -n 1
 dub --version
+ldmd2 --version


The install script sets $DC (ldc2) and $DMD (ldmd2).
Note that these commands are only necessary because dmd is built freshly from source here. I'm about to add this tools repo to the Jenkins Project Tester (dlang/ci#130), so that the test suite will be run on every PR at dmd,druntime and phobos which in other words means that we don't need to build the latest dmd here and can just use $DC from Travis.

FYI here's the PR to dlang/tools: #298

WebDrake · 2018-01-24T11:23:53Z

Travis will generate a Cartesian product out of these.

No, that kind of Cartesian-product outcome is exactly what I do not want. That would involve rdmd being built and then tested with all these different compilers (with each in turn being treated as the default compiler). That doesn't validate what needs validating. If that was sufficient I'd have just extended the build matrix to start with, and not bothered adding a --test-compilers flag to rdmd_test.

What I want is that rdmd should be built as it is intended to be released (i.e. with dmd) and then tested with dmd, gdmd and ldmd2 (as supplied by the rdmd_test --test-compilers option). In other words it's about testing that the released rdmd will work with all the compilers we want, if we ask it to use them with the --compiler option.

So that means that:

build matrix should be as it currently is (i.e. just 32- or 64-bit DMD)
the test images should include LDC and GCD (the latter including the gdmd wrapper script) so that these are available to test with (but NOT used to build rdmd with)
the invocation of make test should specify RDMD_TEST_COMPILERS=dmd,gdmd,ldmd2 (which will determine how --test-compilers is specified when rdmd_test is invoked from the test_rdmd target)

wilzbach · 2018-01-24T11:34:21Z

the test images should include LDC and GCD (the latter including the gdmd wrapper script) so that these are available to test with (but NOT used to build rdmd with)

~/dlang/install.sh is available on Travis, i.e.

source $(~/dlang/install.sh install -a ldc)
source $(~/dlang/install.sh install -a gdc)

The installer will add their binary path to PATH and , but every activation script will overwrite $DC and $DMD - though it's saved and restores when you deactivate.
But words are ambiguous, code isn't:

deactivate() {
    export PATH="$_OLD_D_PATH"
    export LIBRARY_PATH="$_OLD_D_LIBRARY_PATH"
    export LD_LIBRARY_PATH="$_OLD_D_LD_LIBRARY_PATH"
    export PS1="$_OLD_D_PS1"

    unset _OLD_D_PATH
    unset _OLD_D_LIBRARY_PATH
    unset _OLD_D_LD_LIBRARY_PATH
    unset _OLD_D_PS1
    unset DMD
    unset DC
    unset -f deactivate
}

_OLD_D_PATH="${PATH:-}"
_OLD_D_LIBRARY_PATH="${LIBRARY_PATH:-}"
_OLD_D_LD_LIBRARY_PATH="${LD_LIBRARY_PATH:-}"
_OLD_D_PS1="${PS1:-}"

export PATH="/home/seb/dlang/ldc-1.1.0/bin${PATH:+:}${PATH:-}"
export LIBRARY_PATH="/home/seb/dlang/ldc-1.1.0/lib${LIBRARY_PATH:+:}${LIBRARY_PATH:-}"
export LD_LIBRARY_PATH="/home/seb/dlang/ldc-1.1.0/lib${LD_LIBRARY_PATH:+:}${LD_LIBRARY_PATH:-}"
export DMD=ldmd2
export DC=ldc2
export PS1="(ldc-1.1.0)${PS1:-}"

the invocation of make test should specify RDMD_TEST_COMPILERS=dmd,gdmd,ldmd2

Ehm why not as a loop?
It probably simplifies things, e.g.:

make -f posix.mak all
for compiler in dmd ldc gdc ; do
    source $(~/dlang/install.sh install -a "$compiler")
    make -f posix.mak test DMD="$DMD"
    deactivate
done

WebDrake · 2018-01-24T13:06:15Z

@wilzbach Thanks for the detailed feedback and suggestions. I'm going to be in a bit of a rush for the rest of today, but just to touch on one point:

make -f posix.mak test DMD="$DMD"

... this line is based on a misunderstanding of how rdmd_test works. rdmd_test first builds an rdmd instance and then runs tests using it. The DMD specified to the make test target is used as the build compiler by rdmd_test (i.e. it's used to build the test rdmd instance).

We want to specify the test compilers that rdmd_test will ask its test rdmd to use when running the test suite.

It should however be possible to use something similar to the approach you suggest. I'll try to throw something together at some point.

WebDrake · 2018-02-01T01:10:42Z

Just a ping to note -- I'm a bit distracted right now but will try to follow up on this some time soon. Thanks for patience!

WebDrake · 2018-02-01T01:13:39Z

The installer will add their binary path to PATH and , but every activation script will overwrite $DC and $DMD - though it's saved and restores when you deactivate.

(1) it would be nice to be able to install without overwriting $DC and $DMD (maybe an --install-only flag for install.sh?)

(2) does this mean that if I do:

source $(~/dlang/install.sh install -a ldc)
deactivate
source $(~/dlang/install.sh install -a gdc)
deactivate

... I will wind up with both LDC and GDC still installed, but NOT set as $DC and $DMD?

WebDrake · 2018-02-01T01:18:50Z

Let's try that last idea out. I've pushed a fixup patch; if it works, I'll add GDC, and then squash everything down to a clean and rebased patchset.

PetarKirov · 2018-02-01T07:07:10Z

@WebDrake if you don't want any environment variables changes, just don't pass the activate flag (then there will be no need to deactivate as well). install.sh install <compiler-version> will only download the compiler. See https://dlang.org/install#install for more info.

PetarKirov · 2018-02-01T07:08:59Z

travis.sh

@@ -27,9 +27,13 @@ fi

 install_digger

+source $(~/dlang/install.sh install -a ldc)
+deactivate


Replace with simply:

~/dlang/install.sh install ldc

WebDrake · 2018-02-01T23:15:09Z

Rebased on master to resolve conflicts, and tweaked the install commands as suggested. I've also added GDC to the list of installed and tested compilers.

WebDrake · 2018-02-01T23:19:50Z

Ho hum. Looks like the lack of -a means that not only are $DMD and $DC not set, but that the downloaded compilers are not added to the path.

WebDrake · 2018-02-02T00:15:09Z

OK, a few abuses of Travis later (sorry!), I've got something that's working for DMD and LDC.

It looks like GDC (gdmd) has some issues; see https://travis-ci.org/dlang/tools/jobs/336351267 where one of the rdmd_test commands fails in a rather bizarre way. I'll try to identify what's going on there, but in the meantime, this version should at least take some first steps in testing with multiple D compilers.

WebDrake · 2018-02-02T00:28:11Z

Looking closer, it looks like the problem is not with GDC but with rdmd.d. It insists on manually adding a terminating ; to whatever statement it is given, even if it already has one (which all the --eval statements issued by rdmd_test.d do). GDC very sensibly objects to this, since (for example)

writeln(`eval_works`);;

... is not a single writeln, but a writeln statement followed by an empty statement.

The whole way that eval is handled inside rdmd.d is a bit flaky anyway (the construction of the program string from the eval array of strings really ought to be done in a separate, unittested function), so it looks like it would be a good idea to fix that first before adding gdmd to the compiler list.

Things get fun when you start testing with multiple compilers, don't they? :-P

WebDrake · 2018-02-02T00:28:59Z

Discussion aside: I suggest re-reviewing this PR as it stands (since it now works in terms of its original goals), and following up separately on getting GDC into the test compiler list.

WebDrake · 2018-02-02T00:34:10Z

posix.mak

@@ -110,7 +112,7 @@ test_tests_extractor: $(ROOT)/tests_extractor
 	$< -i ./test/tests_extractor/iteration.d | diff - ./test/tests_extractor/iteration.d.ext

 test_rdmd: $(ROOT)/rdmd_test $(ROOT)/rdmd
-	$< --compiler=$(abspath $(DMD)) -m$(MODEL)
+	$< --compiler=$(abspath $(DMD)) -m$(MODEL) -v --test-compilers=$(RDMD_TEST_COMPILERS)


On consideration, I think it could be useful to keep the verbose call to rdmd_test for CI at least. But it's a bit intrusive how it's done here, just hardcoding the -v. Any thoughts from anyone else?

If we decide to keep it as-is, I should probably tweak the last commit message.

Are you looking for something like this:

VERBOSE=0 ... ifeq ($(VERBOSE),1) override VERBOSE_FLAGS:=-v fi ... $< ... $(VERBOSE_FLAGS)

in Travis you can set VERBOSE=1

But it's a bit intrusive how it's done here

I don't think anyone would care. Testsuites are supposed to be chatty and emit a lot of messages.

Yes, I can do something like that. I was not going to because I'd assumed the last patch would just be something to drop after things had been finalized. But given that the output seems useful, I think I'll revise things accordingly.

wilzbach · 2018-02-02T01:00:51Z

OK, a few abuses of Travis later (sorry!)

No need to be sorry about this - that's what CIs are for!

Discussion aside: I suggest re-reviewing this PR as it stands (since it now works in terms of its original goals), and following up separately on getting GDC into the test compiler list.

Yeah, compatibility with GDC was never on the supported list before, so that's more than reasonable.

WebDrake · 2018-02-02T01:01:58Z

Yeah, compatibility with GDC was never on the supported list before, so that's more than reasonable.

Well, TBH I think what's been uncovered is a bug in rdmd. I'll write something up about it tomorrow.

wilzbach · 2018-02-02T01:05:33Z

travis.sh

+
+~/dlang/install.sh list
+
+LDMD2=$(find ~/dlang -type f -name "ldmd2")


I think I should add a flag to the installer script that allows to get the binary path or main binary. What would be more useful?

--print-binary-dir

--print-dc

--print-dmd

Or maybe simply all three of them?

To my mind what would be most useful would be a way for list to list the full compiler paths rather than just the compiler names and versions.

i.e. to be able to do install.sh list --full-path or something like that.

I wouldn't make any of these options a blocker or requirement for this PR, though. The find solution is crude, but it does work.

This patch replaces the old ad-hoc generation of `--eval` or `--loop` program code with calls to the new `makeEvalCode` function. Besides improving separation of concerns, this also ensures that the generated code will never have a trailing blank statement (i.e. two `;` separated at most by whitespace). This should prevent issues when using `--eval` or `--loop` with a `--compiler` choice that is fussy about such things; see dlang#297 (comment) for an example.

WebDrake · 2018-02-04T16:34:07Z

Rebased on master and reworked with 2 key changes:

GDC is now included in the compilers tested with rdmd_test
verbose output of rdmd_test is now controlled by a VERBOSE_RDMD_TEST variable in posix.mak

Overall diff from previous version of PR:

diff --git a/posix.mak b/posix.mak
index 28068bb..a93eb8c 100644
--- a/posix.mak
+++ b/posix.mak
@@ -8,6 +8,12 @@ DUB=dub
 
 RDMD_TEST_COMPILERS = $(abspath $(DMD))
 
+VERBOSE_RDMD_TEST=0
+
+ifeq ($(VERBOSE_RDMD_TEST), 1)
+       override VERBOSE_RDMD_TEST_FLAGS:=-v
+endif
+
 WITH_DOC = no
 DOC = ../dlang.org
 
@@ -112,7 +118,9 @@ test_tests_extractor: $(ROOT)/tests_extractor
        $< -i ./test/tests_extractor/iteration.d | diff - ./test/tests_extractor/iteration.d.ext
 
 test_rdmd: $(ROOT)/rdmd_test $(ROOT)/rdmd
-       $< --compiler=$(abspath $(DMD)) -m$(MODEL) -v --test-compilers=$(RDMD_TEST_COMPILERS)
+       $< --compiler=$(abspath $(DMD)) -m$(MODEL) \
+          --test-compilers=$(RDMD_TEST_COMPILERS) \
+          $(VERBOSE_RDMD_TEST_FLAGS)
        $(DMD) $(DFLAGS) -unittest -main -run rdmd.d
 
 test: test_tests_extractor test_rdmd
diff --git a/travis.sh b/travis.sh
index 7e043c1..968ba39 100755
--- a/travis.sh
+++ b/travis.sh
@@ -7,7 +7,10 @@ set -uexo pipefail
 
 ~/dlang/install.sh list
 
+GDMD=$(find ~/dlang -type f -name "gdmd")
 LDMD2=$(find ~/dlang -type f -name "ldmd2")
 
 make -f posix.mak all DMD=$(which dmd)
-make -f posix.mak test DMD=$(which dmd) RDMD_TEST_COMPILERS=dmd,$LDMD2
+make -f posix.mak test DMD=$(which dmd) \
+    RDMD_TEST_COMPILERS=dmd,$GDMD,$LDMD2 \
+    VERBOSE_RDMD_TEST=1

WebDrake · 2018-02-04T16:41:49Z

How interesting. This test is failing with gdmd:

tools/rdmd_test.d

Line 504 in a8d282e

res = execute(rdmdArgs ~ ["-L-L" ~ srcDir, mainSrcName]);

It looks like there are two main possibilities here: either pragma(lib, ...) is not being handled correctly, or the -L-L flag is not being handled right by gdmd.

WebDrake · 2018-02-04T16:45:47Z

This is gdmd's documented help for -L: https://github.com/D-Programming-GDC/GDMD/blob/4fc2a4de48e14e1efa0d203c6a414beebdfef1ec/dmd-script#L139

WebDrake · 2018-02-04T16:47:07Z

Let's see if --chatty output from rdmd gives us any bigger hints what is going on here.

WebDrake · 2018-02-04T17:02:43Z

The failing rdmd call arises from:

spawn ["/home/travis/dlang/gdc-4.8.5/bin/gdmd", "-m64", "-L-LrdmdTest", "-v", "-o-", "rdmdTest/main.d", "-IrdmdTest"]

These two lines of verbose GDC output seem particularly relevant:

LIBRARY_PATH=/home/travis/dlang/gdc-4.8.5/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/:/home/travis/dlang/gdc-4.8.5/bin/../lib/gcc/:/home/travis/dlang/dmd-2.078.1/linux/lib64/../lib64/:/home/travis/dlang/gdc-4.8.5/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../../lib64/:/lib/x86_64-linux-gnu/:/lib/../lib64/:/usr/lib/x86_64-linux-gnu/:/home/travis/dlang/dmd-2.078.1/linux/lib64/:/home/travis/dlang/gdc-4.8.5/bin/../lib/gcc/x86_64-unknown-linux-gnu/4.8.5/../../../:/lib/:/usr/lib/
COLLECT_GCC_OPTIONS='-m64' '-v' '-fsyntax-only' '-I' 'rdmdTest' '-c' '-o' 'main.o' '-mtune=generic' '-march=x86-64'

WebDrake · 2018-02-04T17:03:42Z

... in other words, it looks like the LIBRARY_PATH is not correctly set. I'll look into this further.

WebDrake · 2018-02-04T17:04:49Z

@ibuclaw can you shed any light on what's happening here? (See #297 (comment) and later comments.)

WebDrake · 2018-02-04T17:40:24Z

BTW, is the installed GDC version really correct?

gdc-4.8.5

That seems HUGELY out of date with the current state of things. Even my 2-year-old Ubuntu LTS install has a more recent GDC than that!

wilzbach · 2018-02-04T17:42:52Z

We just call

curl -L http://gdcproject.org/downloads/LATEST

Ses also: dlang/installer#251

wilzbach · 2018-02-04T17:43:26Z

Are you sure you don't want to move adding support for gdc to another PR?

WebDrake · 2018-02-04T17:44:33Z

Are you sure you don't want to move adding support for gdc to another PR?

It's probably a good idea, but let's give it a day or two. There's no harm in just trying to understand what is happening here.

The `posix.mak` makefile has been updated to allow the user to set a new `RDMD_TEST_COMPILERS` variable (which by default is set to the absolute path of the compiler used to build all the D tools, including rdmd). This can be used locally along the lines of: make -f posix.mak test RDMD_TEST_COMPILERS=dmd,gdmd,ldmd2 but the main point of this patch is to allow CI to start using this new multi-compiler test functionality. This will be added separately in a follow-up patch. No modifications have been made to windows makefiles, since this is not required by Travis CI.

This patch adds a new user-controllable `VERBOSE_RDMD_TEST` option to `posix.mak`, which if set to 1 causes the `test_rdmd` target to pass the `-v` flag to `rdmd_test`: make -f posix.mak test_rdmd VERBOSE_RDMD_TEST=1 This should provide an easy way for users to request verbose output from multi-compiler tests of `rdmd`. This can be helpful when tracking down exactly what `rdmd` invocation failed during testing.

ibuclaw · 2018-02-04T20:07:05Z

It looks like there are two main possibilities here: either pragma(lib, ...) is not being handled correctly, or the -L-L flag is not being handled right by gdmd.

From what I recall, -L is replaced with -Wl,, that should not be a problem here.

pragma(lib) for sure is not supported, as there is no way the program compiling the source module can communicate this to the program invoking the compiler, assembler, and ultimately linker.

Years ago I had thought maybe adding the information to a special object section which is read by the driver would be the right approach (attempt at implementing), but no one has said what the correct behaviour should be (i.e: what happens if you import a module that has pragma(lib)), and the spec says nothing more than "implementation defined", which is another phrase for "not compatible between compilers".

WebDrake · 2018-02-04T20:12:15Z

pragma(lib) for sure is not supported, as there is no way the program compiling the source module can communicate this to the program invoking the compiler, assembler, and ultimately linker.

This is the key problem, then. These tests assume that pragma(lib) is supported.

I'm not sure what the best way is to proceed with this: perhaps to just automatically exclude these tests if gdmd is the compiler?

WebDrake · 2018-02-04T20:20:48Z

Long run, it's probably a good idea to find a way to separate out the tests better so that we have a test suite that can:

be run with an arbitrary existing rdmd instance and specified D compiler;
provide options for tests to skip, for any given invocation (so that when invoking it to use gdmd as the test compiler, we can manually specify, from CI setup, that pragma(lib) tests are to be excluded, rather than hardcoding this in the test suite itself.

WebDrake · 2018-02-05T00:06:25Z

A further note: it looks like the rdmd_test entry with --include=std among the rdmd arguments also fails for gdmd. Exactly why is not clear to me at this stage, but it looks like something is stopping gdmd from including /tmp/std/foo.d in the build.

This patch is something of a proof-of-concept for using rdmd_test's new `--test-compilers` flag to test rdmd using multiple different compilers. The `install.sh` script is used to install LDC (including the DMD-alike `ldmd2` compiler interface interface), and the `make -f posix.mak test` call is updated to specify both `dmd` and `ldmd2` as test compilers for the `rdmd_test` test suite. A little trickery with `find` is necessary to work around the problem that the install location of` ldmd2` is not added to `PATH`. The `VERBOSE_RDMD_TEST` flag is set to ensure that the Travis build logs will contain a full summary of the individual `rdmd` calls from the test suite, for each of the test compilers. This should probably not be the final form for setting up multi compiler tests, but serves as an initial stage which should at a minimum flag up issues like that observed with dlang#271. While in development it has already shown up at least one `rdmd` issue (fixed in dlang#303), so we can reasonably hope that it will prevent others from arising in the first place.

WebDrake · 2018-02-05T21:47:01Z

I've updated to remove GDC from the test compilers for this PR, and to drop the temporary test patch used to try to debug the GDC issues. The important changes:

diff --git a/travis.sh b/travis.sh
index 968ba39..501a9df 100755
--- a/travis.sh
+++ b/travis.sh
@@ -2,15 +2,13 @@
 
 set -uexo pipefail
 
-~/dlang/install.sh install gdc
 ~/dlang/install.sh install ldc
 
 ~/dlang/install.sh list
 
-GDMD=$(find ~/dlang -type f -name "gdmd")
 LDMD2=$(find ~/dlang -type f -name "ldmd2")
 
 make -f posix.mak all DMD=$(which dmd)
 make -f posix.mak test DMD=$(which dmd) \
-    RDMD_TEST_COMPILERS=dmd,$GDMD,$LDMD2 \
+    RDMD_TEST_COMPILERS=dmd,$LDMD2 \
     VERBOSE_RDMD_TEST=1

I'll follow up with GDC in a separate PR at some later time, but this should now be good for final review and (hopefully) merge.

wilzbach

LGTM. There is one thing we forgot for now: ideally dlang/CI executes the same project as defined in the Travis YAML, but I think testing LDC compatibility is a very good reason to just keep the two hard-coded lines at dlang/CI

Thanks for working on this!

wilzbach · 2018-02-06T01:27:34Z

posix.mak

@@ -6,6 +6,14 @@ DRUNTIME_PATH = ../druntime
 PHOBOS_PATH = ../phobos
 DUB=dub

+RDMD_TEST_COMPILERS = $(abspath $(DMD))
+
+VERBOSE_RDMD_TEST=0


That isn't strictly needed, but a good practice nevertheless.

wilzbach · 2018-02-06T01:29:27Z

travis.sh

+
+make -f posix.mak all DMD=$(which dmd)
+make -f posix.mak test DMD=$(which dmd) \
+    RDMD_TEST_COMPILERS=dmd,$LDMD2 \


Always use quotes for variables ( I recommend the shellcheck linter)

You were quick, I'd have fixed that :-P

Travis always uses the same paths as do we on the installer side, so it wasn't really blocking this PR ;-)
(And I generally prefer to avoid PRs getting stalled because of such tiny nits).
Thanks for the quick follow-up.

WebDrake · 2018-02-06T01:47:02Z

Thanks for working on this!

Thank Don, who suggested to me that I should make this happen ;-) It's been fun working on this; thanks for all the careful review and feedback.

I'll follow up with GDC some time in the next days. As we've seen it may be quite involved to actually get that working, but we'll probably discover some useful stuff about assumptions that rdmd makes about the D compiler.

ibuclaw · 2018-02-06T08:06:38Z

A further note: it looks like the rdmd_test entry with --include=std among the rdmd arguments also fails for gdmd. Exactly why is not clear to me at this stage, but it looks like something is stopping gdmd from including /tmp/std/foo.d in the build.

As discussed, it may be that rdmd makes an assumption about where verbose messages are being sent to. This is stderr for gdc as stdout is reserved by -pipe.

The `--eval` and `--loop` flags allow the user to specify small snippets of code that are placed inside the body of an auto-generated `main` that is written to a file and evaluated. The existing implementation uses two separate string concatenations for the two different cases. Besides the code duplication, this means that the resulting code generation cannot be tested. A further more subtle issue arises from the existing implementation's insistence on adding a closing `;` to the generated code. While this takes care of the careless user who forgets to write one themselves, it means that if the user does close their statements correctly, we end up with an empty statement on the end of the code to be evaluated. While some D compilers don't care about this, some object: dlang#297 (comment) ... meaning that `rdmd` will fail to successfully evaluate the code. This patch therefore implements two new code functions to generate the required code. The first, `innerEvalCode`, takes care of the innermost code (the body of either the `main` function for `--eval`, or the loop for `--loop`), adding a terminating `;` if one is not present, but not doing so if one is already in place. The second, `makeEvalCode`, takes two parameters: the `string[]` of code snippets to be evaluated, and a boolean `Flag` indicating whether or not this is the body of a loop (in other words, whether this code should be generated for the `--eval` flag or the `--loop` flag). Unittests have been provided with both to ensure that they produce the expected outcomes.

This patch replaces the old ad-hoc generation of `--eval` or `--loop` program code with calls to the new `makeEvalCode` function. Besides improving separation of concerns, this also ensures that the generated code will never have a trailing blank statement (i.e. two `;` separated at most by whitespace). This should prevent issues when using `--eval` or `--loop` with a `--compiler` choice that is fussy about such things; see dlang#297 (comment) for an example.

wilzbach reviewed Jan 23, 2018

View reviewed changes

wilzbach reviewed Jan 24, 2018

View reviewed changes

WebDrake mentioned this pull request Jan 24, 2018

Remove digger setup and use the compiler from Travis #298

Merged

PetarKirov reviewed Feb 1, 2018

View reviewed changes

WebDrake force-pushed the test-with-ldmd2 branch from 92116b0 to 4dfea7e Compare February 1, 2018 23:14

WebDrake force-pushed the test-with-ldmd2 branch from 9ed68ea to fd560a3 Compare February 2, 2018 00:12

WebDrake commented Feb 2, 2018

View reviewed changes

wilzbach reviewed Feb 2, 2018

View reviewed changes

WebDrake force-pushed the test-with-ldmd2 branch from fd560a3 to cc2348b Compare February 4, 2018 16:32

WebDrake added 2 commits February 4, 2018 19:57

WebDrake force-pushed the test-with-ldmd2 branch from 1a17d0a to cdd576d Compare February 5, 2018 21:44

wilzbach approved these changes Feb 6, 2018

View reviewed changes

wilzbach merged commit 79936af into dlang:master Feb 6, 2018

WebDrake deleted the test-with-ldmd2 branch February 6, 2018 01:38

Geod24 mentioned this pull request Apr 9, 2020

rdmd incorrectly assumes that the D compiler outputs dependency info to stdout #392

Open


		~/dlang/install.sh list

		LDMD2=$(find ~/dlang -type f -name "ldmd2")

Use multiple different test compilers when invoking rdmd_test from Travis CI #297

Use multiple different test compilers when invoking rdmd_test from Travis CI #297

Conversation

WebDrake commented Jan 23, 2018

dlang-bot commented Jan 23, 2018

Bugzilla references

WebDrake commented Jan 23, 2018

WebDrake commented Jan 23, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Jan 23, 2018

WebDrake commented Jan 24, 2018

WebDrake commented Jan 24, 2018

WebDrake commented Jan 24, 2018

wilzbach commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WebDrake commented Jan 24, 2018

wilzbach commented Jan 24, 2018

WebDrake commented Jan 24, 2018

WebDrake commented Feb 1, 2018

WebDrake commented Feb 1, 2018

WebDrake commented Feb 1, 2018

PetarKirov commented Feb 1, 2018

Choose a reason for hiding this comment

WebDrake commented Feb 1, 2018

WebDrake commented Feb 1, 2018

WebDrake commented Feb 2, 2018

WebDrake commented Feb 2, 2018

WebDrake commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wilzbach commented Feb 2, 2018

WebDrake commented Feb 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

wilzbach commented Feb 4, 2018

wilzbach commented Feb 4, 2018

WebDrake commented Feb 4, 2018

ibuclaw commented Feb 4, 2018 • edited Loading

WebDrake commented Feb 4, 2018

WebDrake commented Feb 4, 2018

WebDrake commented Feb 5, 2018

WebDrake commented Feb 5, 2018

wilzbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WebDrake commented Feb 6, 2018

ibuclaw commented Feb 6, 2018

ibuclaw commented Feb 4, 2018 •

edited

Loading