Page MenuHomePhabricator

Driver: Better detection of mingw-gcc
ClosedPublic

Authored by martell on Nov 25 2015, 9:26 PM.

Details

Summary

As discussed after r253898 here is the better gcc detection.

Diff Detail

Repository
rL LLVM

Event Timeline

martell updated this revision to Diff 41207.Nov 25 2015, 9:26 PM
martell retitled this revision from to Driver: Better detection of mingw-gcc.
martell updated this object.
martell added a reviewer: yaron.keren.
martell added a subscriber: cfe-commits.
martell updated this revision to Diff 41208.Nov 25 2015, 9:28 PM
martell updated this object.

Removed the break that I didn't think was needed
Can be re-added ??

Will added testcases

@ismail can you test this for your setup please? :)

yaron.keren edited edge metadata.Nov 25 2015, 10:18 PM

This always searches for something-gcc and then discards the result if sysroot was provided, which is a waste.
Move the searching to a helper function and then it can be done only if sysroot was not provided, as it is now.

The break is needed to avoid looking for mingw32-gcc after x86_64-w64-mingw32-gcc was already found. It's a wasted search.

martell updated this revision to Diff 41209.Nov 25 2015, 10:44 PM
martell edited edge metadata.

Updated to reflect feedback

I'm not too sure what todo for testcases on this one because they already exist for the most part.
I ran the test suite and they all pass.

I would have to create a mingw32-gcc and {armv7|i686|x86_64}-w64-mingw32-gcc and chmod+x for the testcase.
Is that safe ?

Tested on openSUSE and it works. Thanks!

findGccDir() can return llvm::ErrorOr<std::string> and then all Base assignments happen at the same if-elseif-else:

if (getDriver().SysRoot.size())
  Base = getDriver().SysRoot;
else if (llvm::ErrorOr<std::string> GPPName = findGccDir())
 Base = llvm::sys::path::parent_path(
        llvm::sys::path::parent_path(GPPName.get()));
else
  Base = llvm::sys::path::parent_path(getDriver().getInstalledDir());

Then, findGccDir() is actually findGcc().

About tests, adding empty script "gcc" with x set in the directory structure, and adding the directory to the path at start of the LIT test may work. It will fail on Windows so try this with a new test file so we can XFAIL:windows only the new one if required.

ismail added a comment.Dec 9 2015, 8:26 AM

Martell, any update on this?

hi ismail,
I will tidy up for review closer towards the weekend where i can work on out of work stuff

martell updated this revision to Diff 42933.Dec 15 2015, 3:55 PM

Sorry I was a bit slow getting around to this.

Will do testcases if Yaron thinks this is okay

yaron.keren accepted this revision.Dec 16 2015, 12:54 PM
yaron.keren edited edge metadata.

LGTM with some tests cases.

This revision is now accepted and ready to land.Dec 16 2015, 12:54 PM
ismail added a comment.Jan 7 2016, 1:22 AM

LLVM 3.8 will be branched in a week, if we can't get this in before then I'll revert r253898 before branching and apply it again after branching done.

Reverted in r257468.

yaron.keren resigned from this revision.Mar 30 2017, 12:07 PM
This revision now requires review to proceed.Mar 30 2017, 12:07 PM
martell updated this revision to Diff 95292.EditedApr 14 2017, 6:45 AM
martell edited the summary of this revision. (Show Details)
martell added a reviewer: yaron.keren.
martell added a subscriber: yaron.keren.

After the revert I left this patch go to pick up some other patch work.
Seen as we are not anywhere near the next release this seem like a good time to pick it up again.
Also got a reminder when yaron resigned

Updated to master.
@ismail can you confirm this update works for your custom opensuse setup on master.
@yaron.keren I can't do a non flaky test case for this it seems because on windows we need .exe extension and not on unix.
Also we need the test case to be able to set the PATH variable to be able to test this fully.
I can't find any example that lets us set the path variable? Help welcome :)

I would like to consider this as working because it passes all the existing Driver testcases in test/Driver/mingw.cpp

To clarify the most import part about actually being able to test this is the being able to modify the PATH variable for windows and unix hosts in the test case.
The program prefix is less of a problem because I can double down with the same empty file one with .exe and one without.
Just thought that I would note it incase someone has ran into this already.

yaron.keren edited edge metadata.

Adding Mateusz and Reid.

mati865 accepted this revision.Apr 15 2017, 7:33 AM

Looks fine for MinGW on Windows.

This revision is now accepted and ready to land.Apr 15 2017, 7:33 AM