This is an archive of the discontinued LLVM Phabricator instance.

[clang][driver] fix to correctly set devtoolset on RHEL
ClosedPublic

Authored by quinnp on Jun 8 2022, 8:06 AM.

Details

Summary

This patch correctly sets the devtoolset on RHEL.

Diff Detail

Event Timeline

quinnp created this revision.Jun 8 2022, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 8:06 AM
quinnp requested review of this revision.Jun 8 2022, 8:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2022, 8:06 AM
nemanjai added inline comments.Jun 8 2022, 8:13 AM
clang/lib/Driver/ToolChains/Gnu.cpp
2152

I believe this will cause a failure with the unit test added in the original patch.

And presumably since the unit test tests for paths that don't include /root/usr, there are real world toolsets that also don't use that suffix. So we should presumably add both Prefixes.

2157–2159

Can we not just change this to something like:

if (ChosenToolsetVersion > 0) {
  Prefixes.push_back(ChosenToolsetDir);
  Prefixes.push_back(ChosenToolsetDir + "/root/usr");
}
quinnp updated this revision to Diff 435239.Jun 8 2022, 10:19 AM

Addressing review comments.

quinnp marked 2 inline comments as done.Jun 8 2022, 10:20 AM

Thank you @nemanjai! I've updated the patch based on your suggestion and tested it for both the existing testcase and the RHEL buildbot failure.

Do you have more authoritative answer when /root/usr is used and when it isn't?
I'd wish that we have comments for them.

This change also needs a unit test.

Do you have more authoritative answer when /root/usr is used and when it isn't?

These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.

I'd wish that we have comments for them.

I agree 100%. Timm, do you know the answer to this and if so, can you propose a comment to that end?

This change also needs a unit test.

+1

Do you have more authoritative answer when /root/usr is used and when it isn't?

These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.

I did not remove that on purpose, so adding it back makes sense to me.

This change also needs a unit test.

+1

Does the existing unit test not break with this change?

quinnp updated this revision to Diff 435914.EditedJun 10 2022, 7:38 AM

Addressing review comments. Removing path without /root/usr suffix and changing unit test to match.

Thanks @tbaeder, @MaskRay, and @nemanjai.

I did not remove that on purpose, so adding it back makes sense to me.

I've updated the patch to remove the path without the /root/usr suffix and exclusively add the path with the root/usr suffix.

This change also needs a unit test.

I've updated the existing test case to match this change.

MaskRay accepted this revision.Jun 10 2022, 8:58 PM

Looks great!

This revision is now accepted and ready to land.Jun 10 2022, 8:58 PM
This revision was landed with ongoing or failed builds.Jun 13 2022, 7:12 AM
This revision was automatically updated to reflect the committed changes.