This is an archive of the discontinued LLVM Phabricator instance.

[MLIR] Fix runner library paths for integration tests
AbandonedPublic

Authored by csigg on Sep 2 2022, 2:12 AM.

Details

Summary

The shared library paths were set to %mlir_integration_test_dir, when they really should be %linalg_test_lib_dir (which I will rename in a separate change).

This happened to work for cmake builds, but is in the way for enabling these tests in bazel.

Diff Detail

Event Timeline

csigg created this revision.Sep 2 2022, 2:12 AM
Herald added a reviewer: ftynse. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a reviewer: aartbik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
csigg requested review of this revision.Sep 2 2022, 2:12 AM
csigg edited the summary of this revision. (Show Details)Sep 2 2022, 2:16 AM
aartbik requested changes to this revision.Sep 2 2022, 9:07 AM

Why can't we have a change that fixes what you need for mlir_integration_test_dir (the right name for this suite) rather than going through this linalg_test_lib_dir renaming first (which really is a misnomer)

This revision now requires changes to proceed.Sep 2 2022, 9:07 AM
csigg added a comment.Sep 2 2022, 9:17 AM

Why can't we have a change that fixes what you need for mlir_integration_test_dir (the right name for this suite) rather than going through this linalg_test_lib_dir renaming first (which really is a misnomer)

%mlir_integration_test_dir is supposed to contain the build artifacts of that package, not all files that are needed by this test suite.

I agree that the %linalg_test_lib_dir is a misnomer. As I said in the description, I will fix that in a separate change.

aartbik added a comment.EditedSep 2 2022, 9:46 AM

Why can't we have a change that fixes what you need for mlir_integration_test_dir (the right name for this suite) rather than going through this linalg_test_lib_dir renaming first (which really is a misnomer)

%mlir_integration_test_dir is supposed to contain the build artifacts of that package, not all files that are needed by this test suite.

I agree that the %linalg_test_lib_dir is a misnomer. As I said in the description, I will fix that in a separate change.

I don't disagree with your need for an improvement :-), apologies for not articulating my concerns more clearly.

Let me state it differently, you are touching 136 now with the guarantee that you will touch the same 136 files again in the near future.
It seems to me we could up with a solution where, with some prep work, you only touch these 136 files once....

Changes in the testing set up are slightly disruptive, as they potentially break new tests that are in flight.
Also, it adds a confusing history to each individual file.

That is why I was asking for a "one touch"s fix for these files.

csigg added a comment.Sep 2 2022, 10:05 AM

Point taken, let me fix the %linalg_test_lib_dir misnomer first.

Point taken, let me fix the %linalg_test_lib_dir misnomer first.

Thanks!

csigg abandoned this revision.Sep 4 2022, 2:11 AM

Subsumed by D133270.