This is an archive of the discontinued LLVM Phabricator instance.

[libcxx] [test] Fix detection of clang-cl when cross compiling
ClosedPublic

Authored by mstorsjo on Mar 5 2021, 2:59 AM.

Details

Summary

When cross compiling, the compiler tool doesn't have a .exe suffix.

Diff Detail

Event Timeline

mstorsjo created this revision.Mar 5 2021, 2:59 AM
mstorsjo edited the summary of this revision. (Show Details)Mar 5 2021, 3:02 AM
mstorsjo added a reviewer: Restricted Project.
mstorsjo added a subscriber: libcxx-commits.
mstorsjo published this revision for review.Mar 5 2021, 3:05 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 3:05 AM
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald Transcript
ldionne accepted this revision.Mar 5 2021, 6:23 AM
This revision is now accepted and ready to land.Mar 5 2021, 6:23 AM

When cross-compiling (I imagine that you build on Linux and test on Windows), it seems logical to be using WindowsRemoteTI, no?
I understand that it's not there and would be almost identical to WindowsLocalTI, but it would make things clearer IMO.

Another thing, wouldn't LinuxRemoteTI will have is_windows set to True when cross-compiling from Windows, and so need the analogical treatment as you do for Windows here?

When cross-compiling (I imagine that you build on Linux and test on Windows), it seems logical to be using WindowsRemoteTI, no?
I understand that it's not there and would be almost identical to WindowsLocalTI, but it would make things clearer IMO.

Yeah maybe, but so far I don't see the benefit in adding essentially a separate duplicate copy of each of these classes, when I so far haven't seen any case where it would warrant a difference between the local and remote versions. (We have one remote version so far, LinuxRemoteTI, and that one is functionally equivalent to LinuxLocalTI.) But I agree that it's an inconsistency that we have one such but not more than that.

Another thing, wouldn't LinuxRemoteTI will have is_windows set to True when cross-compiling from Windows, and so need the analogical treatment as you do for Windows here?

Yes that's true, so maybe we should move all of the implementations of is_windows, is_darwin etc into the platform specific subclasses, and have the base class versions only return false.

Maybe it'd be best if I split this patch into two; one for just fixing clang-cl matching, and one for moving the is_* functions into subclasses?

mstorsjo updated this revision to Diff 328523.Mar 5 2021, 7:26 AM

Split out the target info change, keeping only the clang-cl detection

mstorsjo retitled this revision from [libcxx] [test] Fix cross testing for windows to [libcxx] [test] Fix detection of clang-cl when cross compiling.Mar 5 2021, 7:26 AM
mstorsjo edited the summary of this revision. (Show Details)
curdeius accepted this revision.Mar 5 2021, 7:30 AM

This part definitely LGTM.

For the target infos, I don't know what the best course of action should be, but certainly there's a place for improvement.