-
-
Notifications
You must be signed in to change notification settings - Fork 672
fix building tests with debug compiler #10231
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.
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
Conversation
|
Thanks for your pull request, @rainers! Bugzilla referencesYour PR doesn't reference any Bugzilla issue. If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog. Testing this PR locallyIf you don't have a local development environment setup, you can use Digger to test this PR: dub fetch digger
dub run digger -- build "master + dmd#10231" |
|
We should look into enabling this on a CI (see e.g. #7528), s .t. it doesn't break again. |
6a9b759 to
2a289a3
Compare
test/dshell/sameenv.d
Outdated
| const fromExe = readText(envFromExe); | ||
| const fromRun = readText(envFromRun); | ||
| const fromExe = readOutput(envFromExe); | ||
| const fromRun = readOutput(envFromRun); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@marler8997 This test from #8121 didn't work with a debug compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you fix this issue? I'm not seeing any failures with this test in the CIs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filtering in readOutput is necessary to remove the "DMD 2.087.1-git-1234 DEBUG" message generated by a debug build of dmd. All existing CIs use a release build (i.e. none of the asserts in the compiler is tested), but this PR changes that.
IIRC you were the main contributor of the dshell-tests, so I just wanted to note that these tests should usually use readOutput (or something similar) to process compiler output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW is printing this debug string on every execution really worth it?
It seems to produce more trouble than gain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my other suggestion, this becomes readText(envFromRun).filterCompilerDebugMsg
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW is printing this debug string on every execution really worth it?
It seems to produce more trouble than gain.
I agree, it's quite a nuisance with little benefit.
wilzbach
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot! I'm already pre-approving the CI part of this.
| /** | ||
| read the the given `file` and remove \r and the compiler debug header. | ||
| */ | ||
| string readOutput(string file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of a function to read a file of compiler output, how about a function called 'filterCompilerDebugMsg'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I'll change it.
test/dshell/sameenv.d
Outdated
|
|
||
| const fromExe = readText(envFromExe); | ||
| const fromRun = readText(envFromRun); | ||
| const fromExe = readOutput(envFromExe); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The exe output won't have the debug message to filter out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output still needs to be stripped from \r otherwise the output produces duplicate newlines.
test/dshell/sameenv.d
Outdated
| const fromExe = readText(envFromExe); | ||
| const fromRun = readText(envFromRun); | ||
| const fromExe = readOutput(envFromExe); | ||
| const fromRun = readOutput(envFromRun); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With my other suggestion, this becomes readText(envFromRun).filterCompilerDebugMsg
Does anyone know how libcurl.dll is built? It seems broken since dmd 2.072, earlier versions work. Also the DLL from LDC is ok. @MartinNowak ? |
|
See dlang/phobos#7126 for the std.math failures: |
It's built at dlang/installer. |
Thanks, now I remember. It seems to be related to the version of libcurl: https://issues.dlang.org/show_bug.cgi?id=20120 |
|
Now using libcurl from https://ci.appveyor.com/project/4wil/installer/builds/26627815/artifacts for demonstration |
| if [ ! -f dmd2/README.TXT ]; then | ||
| download "http://downloads.dlang.org/releases/2.x/${HOST_DMD_VERSION}/dmd.${HOST_DMD_VERSION}.windows.7z" dmd2.7z | ||
| 7z x dmd2.7z > /dev/null | ||
| download "https://ci.appveyor.com/api/buildjobs/nogriv1wq32h4jr0/artifacts/libcurl-7.65.3-WinSSL-zlib-x86-x64.zip" libcurl.zip |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't be available forever -> let's use a version from downloads.dlang.org
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-> #10326
also needs #10165 and #10158 from stable.