STORM-977 llmediaplugintest shows up even though -DLL_TESTS:BOOL=OFF has been used


Submitter:	Jonathan Yap	Reviewers
Branch:	2.6	Groups:	viewer
Bugs:	STORM-977	People:
Change Number:	None	Repository:	viewer-development

Description:

llmediaplugintest shows up to be compiled even though -DLL_TESTS:BOOL=OFF has been used on the command line.

This cmake file does not use the call to LL_ADD_PROJECT_UNIT_TESTS that other cmake files do.

LL_ADD_PROJECT_UNIT_TESTS is usually wrapped with if(LL_TESTS)

I could not figure out which lines suppress the inclusion of copy_plugintest_libs and llmediaplugintest into the list of what is to be built so wrapped the entire file around if(LL_TESTS).

If there is some better way to more exactly target these two items please point it out.

Testing Done:

Expand All

> If there is some better way to more exactly target these two items please point it out.

You should be able to get the same effect when wrapping the only place where this file is referenced in a LL_TESTS condition, i.e., change https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-74 from
if (NOT LINUX)
to
if (LL_TESTS AND NOT LINUX)
(and the same for the endif, of course)

Whether that's a better place, I don't know.

Though, I think LL_TESTS is the wrong conditional here, anyway. LL_TESTS is for enabling unit and integration tests. llplugintest however, I have learned on IRC today, seems to be a fully separate program based on and similar to uBrowser that could be used to load and test individual llmediaplugins, would it communicate with them in the same way the viewer does. (Which, according to MichelleZ, it doesn't, thus potentially misleading developers of new plugins.)

It should probably not be built unless explicitly requested, thus a new variable, defaulting to OFF and different from LL_TESTS would suit this much better. Or, just delete the referencing at https://bitbucket.org/lindenlab/viewer-development/src/b0bceb572090/indra/CMakeLists.txt#cl-75 completely, thus unconditionally removing it from the dependencies of the viewer itself, and have those that want to build it explicitly state it as a build target.

Merov Linden Feb. 16, 2011, 10:15 a.m. (Feb. 16, 2011, 10:15 a.m.)

I'm afraid that, if that app is not built by default, it will rapidly rot and become unmaintainable. There is value having it built (even if not used) by LL devs and TC at least (who all build with LL_TESTS on): make sure we don't add viewer dependencies where we don't need any and maintain a clean separation of concerns between plugins and hosts.

Boroondas Gupte Feb. 16, 2011, 12:36 p.m. (Feb. 16, 2011, 12:36 p.m.)

I can see that point. Though, I'd still like a separate variable, if alone to avoid people from mistaking llplugintest for a unit or integration test, like I initially did. Just have that (new) variable default on ON, then, rather than OFF.

Jonathan Yap Feb. 16, 2011, 12:44 p.m. (Feb. 16, 2011, 12:44 p.m.)

New variable name aside, would it make it clearer what this module does if it were renamed to something more obvious?  One person pointed out to me that lltestplugin might be a better choice.

Boroondas Gupte Feb. 16, 2011, 4:02 p.m. (Feb. 16, 2011, 4:02 p.m.)

> New variable name aside, would it make it clearer what this module does if it were renamed to something more obvious?
Yes! Though to what?

> One person pointed out to me that lltestplugin might be a better choice.
That might have been me, back when I thought it was a test plugin, which it apparently isn't, either.

If you want to bypass the entire make file for llplugintest, it seems to me better to do it at the indra/CMakeLists.txt level. There's only one line in there that's relevant to llplugintest. Could you try that out?

Diff
- added Diff r2

Description:

Updated code per Merov's request and Boroondas' suggestion.

Cleaner! :) I'm assuming you built on Windows and it doesn't create weird errors down the road.

Jonathan Yap Feb. 16, 2011, 2:10 p.m. (Feb. 16, 2011, 2:10 p.m.)

I have built it on windows, started the viewer up, walked/flew around a bit.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.