> On Feb. 8, 2011, 10:41 a.m., Boroondas Gupte wrote: > > > 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 wrote: > 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 wrote: > 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.
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. - Jonathan ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://codereview.secondlife.com/r/144/#review347 ----------------------------------------------------------- On Feb. 16, 2011, 11:32 a.m., Jonathan Yap wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://codereview.secondlife.com/r/144/ > ----------------------------------------------------------- > > (Updated Feb. 16, 2011, 11:32 a.m.) > > > Review request for Viewer. > > > Summary > ------- > > 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. > > > This addresses bug STORM-977. > http://jira.secondlife.com/browse/STORM-977 > > > Diffs > ----- > > indra/CMakeLists.txt b0bceb572090 > indra/test_apps/llplugintest/CMakeLists.txt b0bceb572090 > > Diff: http://codereview.secondlife.com/r/144/diff > > > Testing > ------- > > > Thanks, > > Jonathan > >
_______________________________________________ Policies and (un)subscribe information available here: http://wiki.secondlife.com/wiki/OpenSource-Dev Please read the policies before posting to keep unmoderated posting privileges
