This is an archive of the discontinued LLVM Phabricator instance.

[mlir] [test] Add missing tool substitutions
ClosedPublic

Authored by mgorny on Oct 2 2021, 2:56 AM.

Details

Summary

Add missing mlir-capi-*-test tool substitutions in order to fix CAPI
test failures when mlir is not installed yet.

Diff Detail

Event Timeline

mgorny created this revision.Oct 2 2021, 2:56 AM
mgorny requested review of this revision.Oct 2 2021, 2:56 AM
stellaraccident accepted this revision.Oct 2 2021, 7:02 AM
This revision is now accepted and ready to land.Oct 2 2021, 7:02 AM

In which condition is this needed? The description says "when mlir is not installed yet" but the bots or the normal development flow does not include any installation and yet they seem to be working.

Ah it may be more clear with the description in https://reviews.llvm.org/D110992 ; is this about building MLIR standalone against an already installed LLVM?

That is how I read the description and patch (agreed a better phrasing
would help).

Ah it may be more clear with the description in https://reviews.llvm.org/D110992 ; is this about building MLIR standalone against an already installed LLVM?

Yes. Technically, D110992 should also resolve this but I've figured out that since some substitutions are already in place then it's cleaner to keep the complete list here.

mehdi_amini accepted this revision.Oct 3 2021, 10:48 AM

I see! So with D110992 we could just delete this substitution right?

(I like the substitution because when a test fails, I can copy/past the command-line without adjusting it to add the path)

I see! So with D110992 we could just delete this substitution right?

Yes, I think so.

(I like the substitution because when a test fails, I can copy/past the command-line without adjusting it to add the path)

Now I'm getting mixed signals. Do you want to keep them or delete them after all?

Now I'm getting mixed signals. Do you want to keep them or delete them after all?

Sorry for the confusion :)
I was trying to express that even if they aren't strictly needed, they are still useful :)
So yeah feel free to push this revision.

This revision was automatically updated to reflect the committed changes.