> 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.
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. - Boroondas ----------------------------------------------------------- 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
