Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Run rdmd testsuite properly #244

Merged
merged 4 commits into from
Jul 4, 2017
Merged

Run rdmd testsuite properly #244

merged 4 commits into from
Jul 4, 2017

Conversation

wilzbach
Copy link
Member

@wilzbach wilzbach commented Jul 4, 2017

Follow-up to #239 (comment)

This should make (1) it a lot easier to run the test suite properly (after all it's now part of the test Makefile target) and (2) should be a good step into debugging why Travis build succeed.

Currently, however, it fails:

pmake test_rdmd
../dmd/generated/linux/release/64/dmd -I../druntime/import -I../phobos -L-L../phobos/generated/linux/release/64 -m64 -w -de -ofgenerated/linux/64/rdmd_test rdmd_test.d
../phobos/std/parallelism.d(3523): Warning: statement is not reachable
../phobos/std/parallelism.d(3523): Warning: statement is not reachable
make: *** [posix.mak:72: generated/linux/64/rdmd_test] Error 1

I have looked into fixing rdmd_test two weeks ago and the fix for this is still in the Phobos queue: dlang/phobos#5502

CC @CyberShadow

@dlang-bot
Copy link
Contributor

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 wilzbach mentioned this pull request Jul 4, 2017
Merged
@andralex
Copy link
Member

andralex commented Jul 4, 2017

@wilzbach thx!

@wilzbach
Copy link
Member Author

wilzbach commented Jul 4, 2017 •
edited
Loading

@CyberShadow any idea why this happens on the for MODEL=32?

Argh - it seems like we need to rework the testsuite to include $MODEL

@wilzbach
Copy link
Member Author

wilzbach commented Jul 4, 2017

Wohoo this is finally passing on Travis 🎉
I had to explicitly pass the model in the rdmd_test suite though - @CyberShadow @andralex are you okay with this?

CyberShadow
CyberShadow reviewed Jul 4, 2017
View reviewed changes
rdmd_test.d Outdated
@@ -88,65 +91,65 @@ void runTests()
string forceSrc = tempDir().buildPath("force_src_.d");
std.file.write(forceSrc, `void main() { pragma(msg, "compile_force_src"); }`);

res = execute([rdmdApp, compilerSwitch, forceSrc]);
res = execute([rdmdApp, compilerSwitch, modelSwitch, forceSrc]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any instance of compilerSwitch that's not accompanied by an instance of of modelSwitch? If no, then just append model to compilerSwitch.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any instance of compilerSwitch that's not accompanied by an instance of of modelSwitch?

Nope, but as execute excepts an array, we can't do ["dmd", "--compiler=foo -m32", ...].
Btw (a bit OT): is there any reason that execute doesn't allow ranges?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can just break the array and concatenate sub-arrays.

Anyway I looked and rdmdApp always occurs together with compilerSwitch, so I'd refactor those into an array, so that the invocation looks like execute(rdmdArgs ~ [...])

@wilzbach
Copy link
Member Author

wilzbach commented Jul 4, 2017

so I'd refactor those into an array, so that the invocation looks like execute(rdmdArgs ~ [...])

Fair enough. Done.

CyberShadow
CyberShadow reviewed Jul 4, 2017
View reviewed changes
travis.sh
build_digger() {
git clone --recursive https://github.com/CyberShadow/Digger "$DIGGER_DIR"
(cd "$DIGGER_DIR" && rdmd --build-only -debug digger)
}

install_digger() {
$DIGGER build --model=$MODEL "master"
export PATH=$PWD/result/bin:$PATH
export PATH=$PWD/work/result/bin:$PATH
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest pinning Digger to a specific version (as with DScanner) to avoid the breaking changes in Digger's master from affecting tools.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay - will do this in another PR

@dlang-bot dlang-bot merged commit c0ddbcd into dlang:master Jul 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andralex andralex andralex approved these changes

@CyberShadow CyberShadow CyberShadow approved these changes

Assignees
No one assigned
Labels
auto-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@wilzbach @dlang-bot @andralex @CyberShadow