Convert DUB package test into a Makefile test and run it with the proper compiler flags by wilzbach · Pull Request #6999 · dlang/dmd

wilzbach · 2017-07-16T17:29:34Z

Fix-up to #6771 (comment)

I think it's reasonable to test the new DUB package with the newly build D compiler and not the host compiler.

dlang-bot · 2017-07-16T17:29:34Z

Thanks for your pull request, @wilzbach!

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.

wilzbach · 2017-07-16T17:30:21Z

test/Makefile

CC @RazvanN7 - we really need to get this migration finished...

wilzbach · 2017-07-16T17:31:11Z

test/Makefile

--root didn't work & we should get Travis green ASAP:

> DFLAGS="-m64" dub test --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd --force --root=/home/seb/dlang/dmd/test/dub_package DFLAGS="-m64" dub test --compiler=/home/seb/dlang/dmd/generated/linux/release/64/dmd --force --root=/home/seb/dlang/dmd/test/dub_package Failed to invoke the compiler /home/seb/dlang/dmd/generated/linux/release/64/dmd to determine the build platform: { /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(86): Error: undefined identifier `string` /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(7): while evaluating pragma(msg, " \"compiler\": \"" ~ determineCompiler() ~ "\",") "frontendVersion": 2075, "compilerVendor": "Digital Mars D", "platform": [ /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(18): Error: undefined identifier `string` /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(11): while evaluating pragma(msg, " " ~ determinePlatform()) ], "architecture": [ /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(43): Error: undefined identifier `string` /tmp/dub_platform_probe-b1b52c05-da2e-4097-892b-89a1507f1d3a.d(14): while evaluating pragma(msg, " " ~ determineArchitecture()) ], }

Also I decided to avoid overwriting the DFLAGS as the -fPIC by default is lost then.

wilzbach · 2017-07-16T17:35:46Z

travis.sh

I didn't add test_dub to the default all target as it would then by run on the auto-tester as well. While we do want to test this on the long run, I wasn't sure whether dub is available on the auto-tester, so I left testing this open for another PR.

Yeah, that's why I only added it to the Travis CI tests from the beginning.

wilzbach · 2017-07-16T17:43:11Z

CC @CyberShadow @andralex master is currently broken on Travis due to #6771 (comment), so I am raising the awareness for this one ;-)

jacob-carlborg · 2017-07-16T18:26:00Z

test/Makefile

 export RESULTS_DIR=test_results
 export MODEL
 export REQUIRED_ARGS=
+BUILD=release


Is this used?

Yep, I had to do quite a few try to figure out a working setup and in the one you reviewed I temporarily removed the new changes with $(BUILD)

wilzbach · 2017-07-17T00:30:22Z

test/Makefile

+else
+REAL_MODEL=64
+endif
+export DMD=../generated/$(OS)/$(BUILD)/$(REAL_MODEL)/dmd


I already bumped into this before: #6870
This is required as dub doesn't forward the -L-L DFLAGS to the linker (in this case dmd again) and thus we need the dmd.conf, which isn't generated for src/dmd by default anymore.

Linking... /home/seb/dlang/dmd/generated/linux/release/64/dmd -of.dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2075-1833BAD8BC30CEBFE35D7CC6DA0FFC50/dmd-dub-test .dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2075-1833BAD8BC30CEBFE35D7CC6DA0FFC50/dmd-dub-test.o ../../.dub/build/library-$DFLAGS-linux.posix-x86_64-dmd_2075-3768A66D24D1220DBE5011CA9AC1DEA7/libdmd_parser.a ../../.dub/build/library-$DFLAGS-linux.posix-x86_64-dmd_2075-00BFCF605E8D9C274B8361919B5DB43D/libdmd_lexer.a ../../.dub/build/library-$DFLAGS-linux.posix-x86_64-dmd_2075-831C9FFAA175BCEDD4C554013935344A/libdmd_root.a -L--no-as-needed -m64 -defaultlib=libphobos2.so Copying target from /home/seb/dlang/dmd/test/dub_package/.dub/build/application-$DFLAGS-linux.posix-x86_64-dmd_2075-1833BAD8BC30CEBFE35D7CC6DA0FFC50/dmd-dub-test to /home/seb/dlang/dmd/test/dub_package Running ./dmd-dub-test ./dmd-dub-test: error while loading shared libraries: libphobos2.so.0.75: cannot open shared object file: No such file or directory

Wait, so you want to build the Dub library with the built DMD, not the host DMD?

I guess testing that the compiler can auto-bootstrap is interesting, but wouldn't it be easier and more consistent to use the host DMD? Or is that difficult because only dmd's posix.mak knows what it is?

to use the host DMD

We should test both, but this entire issue exits because gdc's fronted is too old and has bugs.

As the goal is to check whether the dub package recipe works we should just use the host compiler and avoid additional complications.

wilzbach · 2017-07-17T00:38:48Z

@CyberShadow instead of discussing in length the hypothetical problem of bootstrapping with an unsecure host, could we maybe move our focus to the fact that Travis is broken on master?
(for some reason only I seem to care about this)

I already had quite some "fun" with the Makefiles and auto-tester here, but I think I finally found a setup that's working and testing the DUB package properly.

CyberShadow · 2017-07-17T00:42:15Z

Sorry Sebastian, I'm currently dealing with a broken dev box at home and replying to emails is all I can do, and this looks like I'd need to sit down and wrap my head around this change to review it properly. If you'd like to submit a PR that simply disables the broken test for now I could merge that now, or someone else could look at this instead.

CyberShadow · 2017-07-17T00:45:56Z

You could of course also revert the bad PR instead of trying to rush in a fix.

wilzbach · 2017-07-17T00:49:12Z

Sorry Sebastian, I'm currently dealing with a broken dev box at home and replying to emails is all I can do,

Didn't know that. If it's related to PIC, then I am pretty annoyed by this as well and would love to update our Makefile scripts to use -fPIC by default everywhere on Posix...

you'd like to submit a PR that simply disables the broken test for now I could merge that now, or someone else could look at this instead.

Sounds like a good idea: #7001

You could of course also revert the bad PR instead of trying to rush in a fix.

Nay, I like DUB support too much for this ;-)

CyberShadow · 2017-07-17T00:58:36Z

If it's related to PIC, then I am pretty annoyed by this as well and would love to update our Makefile scripts to use -fPIC by default everywhere on Posix...

No, actually, @Earnestly on #archlinux let me know of a simple workaround. Just create a cc/gcc wrapper which adds -no-pie, e.g.:

#!/bin/bash
set -eu

next=/usr/sbin/$(basename "$0")

for arg in "$@"
do
	if [[ "$arg" == -L$HOME/data/software/dmd/* || "$arg" == -L$HOME/opt/dmd/* ]]
	then
		exec "$next" -no-pie "$@"
	fi
done

exec "$next" "$@"

Add/symlink this to cc and gcc in your PATH, and old releases up to 2.060 will work again.

…iler flags

CyberShadow · 2017-07-17T03:37:24Z

test/Makefile

 else
 export ARGS=-inline -release -g -O -fPIC
-export DMD=../src/dmd
+# hack around the fact that on auto-tester MODEL isn't set for the testsuite


That doesn't sound right.

https://github.com/braddr/at-client/blob/master/src/do_test_dmd.sh#L33-L37

There's certainly a MODEL parameter there.

Generally, if things can be fixed in the AT scripts, they should be. I think Brad has generally been responsive in merging AT pull requests as long as they don't require more involved things like merging some pull requests simultaneously or manually updating test machines.

As mentioned I have bumped into this before: #6870
And no I don't think that we want to wait for another one or two months here. If this gets fixed at the auto-tester's side, the workaround can easily be removed.

Yes that looks wrong, osmodel.mak will already determine a default MODEL, unless one is explicity specified via make MODEL=64. Not sure what fails for you, but you can use $(error ${MODEL}) to inspect variables.

CyberShadow · 2017-07-17T03:39:35Z

test/Makefile

+else
+REAL_MODEL=64
+endif
+export DMD=../generated/$(OS)/$(BUILD)/$(REAL_MODEL)/dmd


Wait, so you want to build the Dub library with the built DMD, not the host DMD?

I guess testing that the compiler can auto-bootstrap is interesting, but wouldn't it be easier and more consistent to use the host DMD? Or is that difficult because only dmd's posix.mak knows what it is?

jacob-carlborg · 2017-07-17T06:53:15Z

dub.sdl

 license "BSL-1.0"

-targetType "none"
+targetType "library"


I was told by Sönke to not do this #6771 (comment).

It doesn't copy all DFLAGS otherwise.

Who doesn't copy what DFLAGS?

MartinNowak

I'd say let's move this out of the test/Makefile mess and leave it in src/posix.mak as a test_dub target.
Furthermore there is no need to deal with the -conf= complications or raw DFLAGS, just testing that the dub recipe works with the HOST_DMD compiler is sufficient (we shouldn't generate a dmd.conf in the src folder during testing).

MartinNowak · 2017-07-26T19:35:20Z

test/Makefile

+else
+REAL_MODEL=64
+endif
+export DMD=../generated/$(OS)/$(BUILD)/$(REAL_MODEL)/dmd


As the goal is to check whether the dub package recipe works we should just use the host compiler and avoid additional complications.

MartinNowak · 2017-07-26T19:40:01Z

test/Makefile

 else
 export ARGS=-inline -release -g -O -fPIC
-export DMD=../src/dmd
+# hack around the fact that on auto-tester MODEL isn't set for the testsuite


Yes that looks wrong, osmodel.mak will already determine a default MODEL, unless one is explicity specified via make MODEL=64. Not sure what fails for you, but you can use $(error ${MODEL}) to inspect variables.

MartinNowak · 2017-07-26T19:43:46Z

dub.sdl

 license "BSL-1.0"

-targetType "none"
+targetType "library"


Who doesn't copy what DFLAGS?

MartinNowak · 2017-07-26T19:49:04Z

test/Makefile

@@ -97,6 +97,7 @@ QUIET=@
 export RESULTS_DIR=test_results
 export MODEL
 export REQUIRED_ARGS=


BTW, working with exported env parameters is really annoying when reading a log failure and not being able to rerun a command.

wilzbach · 2017-12-09T08:12:24Z

Superseded by #7415 which simply uses the host dmd as suggested.

wilzbach changed the title ~~Convert DUB package into a Makefile test and run it with the proper compiler flags~~ Convert DUB package test into a Makefile test and run it with the proper compiler flags Jul 16, 2017

wilzbach force-pushed the fix-travis branch from 7ee95d1 to 1bb9730 Compare July 16, 2017 17:33

wilzbach commented Jul 16, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 16, 2017

Fix issue 17392 - Add Dub file for the lexer and parser #6771

Merged

wilzbach force-pushed the fix-travis branch from 1bb9730 to 909e186 Compare July 16, 2017 17:38

wilzbach added the Review:Blocking Other Work review and pulling should be a priority label Jul 16, 2017

jacob-carlborg reviewed Jul 16, 2017

View reviewed changes

wilzbach force-pushed the fix-travis branch 2 times, most recently from 863c4f7 to 9cb8b85 Compare July 16, 2017 20:31

wilzbach added the Review:Needs Work label Jul 16, 2017

wilzbach force-pushed the fix-travis branch 9 times, most recently from 0d09528 to 2d5eba5 Compare July 17, 2017 00:09

wilzbach removed the Review:Needs Work label Jul 17, 2017

wilzbach force-pushed the fix-travis branch from 03fb7d6 to b0ec7cb Compare July 17, 2017 00:33

wilzbach mentioned this pull request Jul 17, 2017

Use generated DMD binary instead of src/dmd #6870

Merged

wilzbach requested a review from CyberShadow July 17, 2017 00:35

wilzbach commented Jul 17, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 17, 2017

Temporarily skip testing the DUB package #7001

Merged

wilzbach force-pushed the fix-travis branch from b0ec7cb to 6e766e1 Compare July 17, 2017 00:53

wilzbach force-pushed the fix-travis branch from 6e766e1 to f0ba864 Compare July 17, 2017 02:13

Convert DUB test into a Makefile test and run it with the proper comp…

f0ba864

…iler flags

CyberShadow reviewed Jul 17, 2017

View reviewed changes

jacob-carlborg reviewed Jul 17, 2017

View reviewed changes

wilzbach mentioned this pull request Jul 26, 2017

factor out config.sh script to update config files (VERSION, SYSCONFDIR) #7036

Merged

MartinNowak requested changes Jul 26, 2017

View reviewed changes

MartinNowak reviewed Jul 26, 2017

View reviewed changes

wilzbach mentioned this pull request Dec 9, 2017

Start testing the DMD DUB package on Travis #7415

Merged

wilzbach closed this Dec 9, 2017

wilzbach deleted the fix-travis branch December 9, 2017 08:13

Uh oh!

Conversation

wilzbach commented Jul 16, 2017

Uh oh!

dlang-bot commented Jul 16, 2017

Bugzilla references

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

wilzbach commented Jul 17, 2017

Uh oh!

CyberShadow commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Dec 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MartinNowak Jul 26, 2017 •

edited

Loading