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.

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

issue 18515: Fix it so that neither g++ nor CC is required. #7949

Merged
merged 1 commit into from Feb 24, 2018

Conversation

jmdavis
Copy link
Member

@jmdavis jmdavis commented Feb 24, 2018

It was previously fixed so that cc is used if CC isn't set when
compiling dmd, but the test suite still had g++ hardcoded, so if the
system was set up with clang and not gcc, the dmd test suite barfed
midway through. This fixes it so that it uses c++ if CC isn't set
instead of using g++.

I don't know if this will fix the problem that Brad is having with the 32-bit FreeBSD auto-tester that's forcing him to set CC, but it does fix it so that my 64-bit FreeBSD system can run the dmd test suite without g++.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @jmdavis!

Bugzilla references

Auto-close Bugzilla Severity Description
18515 critical freebsd 11 ships with gcc unable to link 32 bit binaries, dmd uses it by default

It was previously fixed so that cc is used if CC isn't set when
compiling dmd, but the test suite still had g++ hardcoded, so if the
system was set up with clang and not gcc, the dmd test suite barfed
midway through. This fixes it so that it uses c++ if CC isn't set
instead of using g++.
@jmdavis
Copy link
Member Author

jmdavis commented Feb 24, 2018

I don't understand the Win32 failure, since it's not code I touched, and the errors don't seem to match anything on the lines in question. So, I rebased on the latest master in case that would fix it.

@jmdavis
Copy link
Member Author

jmdavis commented Feb 24, 2018

Well, that didn't fix the Windows failures, and the Windows failures make no sense whatsoever

d_do_test.d(571): Error: found `<<` instead of statement
d_do_test.d(574): Error: found `==` instead of statement
d_do_test.d(577): Error: found `>>>` instead of statement

None of those lines have those characters.

@wilzbach
Copy link
Member

I don't understand the Win32 failure, since it's not code I touched,

It is. The testsuite is run on Windows too ;-)
In fact the testsuite is on of the few pieces where the ugly separation into three redundant Makefiles doesn't exist and just GNUMake (gmake) is required.
Thus the error - c++ isn't a thing on Windows.

@jmdavis
Copy link
Member Author

jmdavis commented Feb 24, 2018

Thus the error - c++ isn't a thing on Windows.

g++ isn't either, and the switch statement in question has lines for win32 and win64, so the c++ line shouldn't even be hit on Windows. Regardless, the failure is a compilation failure, not a runtime one. So, the value of a string that isn't being mixed in should be irrelevant.

@wilzbach
Copy link
Member

Sorry I wrote my last message to fast. The errors are due to auto-tester's patching: https://github.com/braddr/at-client/blob/master/src/diff-dmd-win64.diff

I tried to talk @braddr into stopping that (or at least including the patches into the dmd repo) but I failed.

@wilzbach
Copy link
Member

Regardless, the failure is a compilation failure, not a runtime one. So, the value of a string that isn't being mixed in should be irrelevant.

It seems like we wrote at the same time, but yes - it looks like DMD is opening a file with an unresolved merge conflict...

@jmdavis
Copy link
Member Author

jmdavis commented Feb 24, 2018

So, what's the fix then?

@wilzbach
Copy link
Member

So, what's the fix then?

I don't know either, here are my ideas:

a) getting something like braddr/at-client#8 deployed to move away from this patching
b) finding a way to do this without touching the code without running into the merge conflict (probably pretty ugly in this case)

I guess we have to wait for @braddr's input.

@marler8997
Copy link
Contributor

The simple solution would be to include @braddr's patches in the repo right?

@timotheecour
Copy link
Contributor

timotheecour commented Feb 24, 2018 via email

@wilzbach
Copy link
Member

The simple solution would be to include @braddr's patches in the repo right?

Last time I tried this, it still resulted in a merge conflict, because the patch is still applied and patch fails to do so.

how about forking at-client also ?

Only @braddr can deploy things to the auto-tester.

@braddr I see that you added auto-merge here. Do you plan anything in particular? (the tests are still failing due to the merge conflict)

@braddr
Copy link
Member

braddr commented Feb 24, 2018

Giving it tester priority. Without it the tester stops on first failure until it has nothing else better to test. Assuming it passes correctly, I'll pull and update the tester patches to apply correctly. Yes, those are a problem area, no right this moment isn't the right time to address them.

@braddr braddr merged commit bd24655 into dlang:master Feb 24, 2018
@braddr
Copy link
Member

braddr commented Feb 24, 2018

Ok, it's passed on all posix platforms, so merging. Somewhat wreckless, but I'm pretty confident it's ok.

@wilzbach
Copy link
Member

Thanks a lot for the swift action!

Yes, those are a problem area, no right this moment isn't the right time to address them.

Is there ever a right moment? ;-)
I understand that with the FreeBSD fiasco it really isn't a good time, but history is going to repeat itself (and there are a few PRs open being blocked by this), so if you ever come around deploying something like braddr/at-client#8 (or your own solution) I would be very much obliged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants