[PATCH] D61116: [test-suite] MultiSource/Benchmarks/DOE-ProxyApps-C/RSBench: change *CMAKE* target name only

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added a comment.

In D61116#1484835 <https://reviews.llvm.org/D61116#1484835>, @lebedev.ri wrote:

> In D61116#1484567 <https://reviews.llvm.org/D61116#1484567>, @hfinkel wrote:
>
> > In D61116#1483718 <https://reviews.llvm.org/D61116#1483718>, @lebedev.ri wrote:
> >
> > > In D61116#1483599 <https://reviews.llvm.org/D61116#1483599>, @hfinkel wrote:
> > >
> > > > While I'm sympathetic, we have a lot of benchmark names which are short and could conflict with other things.
> > > >  We have xsbench too.
> > >
> > >
> > > Sure, yes. But handling conflicts as they appears isn't really an invalid strategy, either.
> > >  This *only* changes the cmake target name, not the binary name, so this should be virtually unnoticeable,
> > >  unless one directly cares about that cmake target name that is of course.
> >
> >
> > I'm aware, but this is also a slippery slope. You have the upstream project carrying a patch to avoid an uncommon naming conflict with an out-of-tree enhancement. That's something that we've tried, in general, not to do.
> >
> > > 
> > > 
> > >> Either we should solve this uniformly, or not at all.
> > > 
> > > Can you define the scope of `uniformly`? Every `[a-zA-Z][a-zA-Z0-9]bench`?
> >
> > No, I mean making all of the test suite targets have some naming convention that makes conflict less likely. Maybe adding `llvm-ts-` to all of them, for example. Maybe you could build that into the llvm_multisource macro and thus solve the problem for everything at once.
> >
> > > 
> > > 
> > >> You can always carry local patches for name conflicts with out-of-tree things.
> > > 
> > > I *could* but i'm trying to avoid that.
> >
> > I realize. But the way you avoid that is to contribute your benchmark to the test suite, thus making the naming conflict something we figure out how to resolve during code review.
>
>
> rL345166 <https://reviews.llvm.org/rL345166> / https://llvm.org/docs/Proposals/TestSuite.html#rawspeed
>
> In D61116#1484741 <https://reviews.llvm.org/D61116#1484741>, @Meinersbur wrote:
>
> > llvm_multisource has an option "PREFIX" that can be used to disambiguate target names. However, in my experience when using it collecting metrics does not work as reliable with this options.
> >
> > Why not changing the name of rsbench downstream? Changing upstream to solve downstream conflicts seems to be the wrong direction.
>
>
> The main thing is, i'd //like// for it to be seamlessly integrateable into testsuite, with *no* diff on either side.
>  That is:
>
> - I don't want to change my `rsbench` in upstream, because i *quite* often have to type it, manually.
> - I would //prefer// not to change my `rsbench` (when the time comes to actually upstream it into testsuite), because then that will be a diff from upstream.
> - I don't really see any way to carry **this** (`MultiSource/Benchmarks/DOE-ProxyApps-C/RSBench/CMakeLists.txt`) patch.
>
>   So the question, as i have seen it, is whether test-suite 'consumers' often have to manually type the `rsbench` for the current DOE-ProxyApps-C/RSBench ? I have guessed no, and thus it seemed better to rename *it*.
>
>   It is indeed quite random name clash :/


I understand, but we all need to deal with our own down-stream name clashes. If this problem is worth solving, and I'm happy to believe that it is, then it's not worth solving just for this instance. We cannot have a project where naming choices for anything are restricted by essentially-random downstream conflicts. I realize that this seems like a small change, but it is just not reasonable to make this change for this one case. As I said above, if you want to make the whole issue of conflicts less likely by changing how llvm_multisource defines its targets, then I'm supportive. Just for this one name, I think this is a bad precedent.


Repository:
  rT test-suite

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61116/new/

https://reviews.llvm.org/D61116