date:20110210

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

2011-02-10 Thread Boroondas Gupte


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/147/#review350
---


A good change to avoid heisenbugs.


indra/cmake/GoogleMock.cmake


Don't mix spaces and tabs. (Use spaces here, like in the rest of the file.)



indra/cmake/GoogleMock.cmake


I'd put the
-Wl,--no-as-needed
down on gtest's line, so that the "wrapping" is more obvious. Or maybe even 
use some creative indentation like:
# ...
if (LINUX)
set(GOOGLEMOCK_LIBRARIES 
gmock
-Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs 
gtest.
gtest
-Wl,--as-needed) # continue to use "as-needed" for other libs
elseif(WINDOWS)
# ...


- Boroondas


On Feb. 9, 2011, 10:20 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/147/
> ---
> 
> (Updated Feb. 9, 2011, 10:20 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> From Aleric's patch:
> 
> Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty 
> hard measure.  Not only is it not necessary to do so, it also changes how the 
> viewer is linked depending on a whether or not the tests are compiled and 
> that is not good.
> 
> The reason that this was needed is that libgmock is underlinked (see 
> http://wiki.mandriva.com/en/Underlinking), which is not compatible with 
> -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol 
> that is defined in libgtest.so.o, but -lgtest was not passed to the linker 
> when creating libgmock.so.0:
> 
> Underlinked (no libgtest.so.o):
> $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
>   NEEDED   libstdc++.so.6
>   NEEDED   libm.so.6
>   NEEDED   libc.so.6
>   NEEDED   libgcc_s.so.1
> 
> The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed 
> causing it to be added again. This is only needed on linux, since that the 
> only platform that we use -Wl,--as-needed on. Moreover, we can just set 
> GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed" 
> since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in 
> front of 'things' that don't start with '-', to allow you do pass special 
> flags like this.
> 
> 
> This addresses bug STORM-981.
> http://jira.secondlife.com/browse/STORM-981
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 27dae7b01a81 
>   indra/cmake/GoogleMock.cmake 27dae7b01a81 
>   indra/cmake/LLAddBuildTest.cmake 27dae7b01a81 
> 
> Diff: http://codereview.secondlife.com/r/147/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

2011-02-10 Thread Aleric Inglewood



> On Feb. 10, 2011, 2:02 a.m., Boroondas Gupte wrote:
> > indra/cmake/GoogleMock.cmake, lines 13-14
> > 
> >
> > I'd put the
> > -Wl,--no-as-needed
> > down on gtest's line, so that the "wrapping" is more obvious. Or maybe 
> > even use some creative indentation like:
> > # ...
> > if (LINUX)
> > set(GOOGLEMOCK_LIBRARIES 
> > gmock
> > -Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs 
> > gtest.
> > gtest
> > -Wl,--as-needed) # continue to use "as-needed" for other libs
> > elseif(WINDOWS)
> > # ...

I don't think you can add comments like that in the middle of a SET( ... # ... 
) ? Never tried it though.
If GOOGLEMOCK_LIBRARIES would end up like "gmock -Wl,--no-as-needed # 
VWR-24366: gmock is underlinked, it needs gtest. gtest -Wl,--as-needed" then 
that definitely will break things ;)

@merov: https://codereview.secondlife.com/r/95/ is still open and present on 
this reviewboard. It's about this same patch?


- Aleric


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/147/#review350
---


On Feb. 9, 2011, 10:20 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/147/
> ---
> 
> (Updated Feb. 9, 2011, 10:20 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> From Aleric's patch:
> 
> Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty 
> hard measure.  Not only is it not necessary to do so, it also changes how the 
> viewer is linked depending on a whether or not the tests are compiled and 
> that is not good.
> 
> The reason that this was needed is that libgmock is underlinked (see 
> http://wiki.mandriva.com/en/Underlinking), which is not compatible with 
> -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol 
> that is defined in libgtest.so.o, but -lgtest was not passed to the linker 
> when creating libgmock.so.0:
> 
> Underlinked (no libgtest.so.o):
> $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
>   NEEDED   libstdc++.so.6
>   NEEDED   libm.so.6
>   NEEDED   libc.so.6
>   NEEDED   libgcc_s.so.1
> 
> The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed 
> causing it to be added again. This is only needed on linux, since that the 
> only platform that we use -Wl,--as-needed on. Moreover, we can just set 
> GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed" 
> since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in 
> front of 'things' that don't start with '-', to allow you do pass special 
> flags like this.
> 
> 
> This addresses bug STORM-981.
> http://jira.secondlife.com/browse/STORM-981
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 27dae7b01a81 
>   indra/cmake/GoogleMock.cmake 27dae7b01a81 
>   indra/cmake/LLAddBuildTest.cmake 27dae7b01a81 
> 
> Diff: http://codereview.secondlife.com/r/147/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

2011-02-10 Thread Aleric Inglewood



> On Feb. 10, 2011, 2:02 a.m., Boroondas Gupte wrote:
> > indra/cmake/GoogleMock.cmake, lines 13-14
> > 
> >
> > I'd put the
> > -Wl,--no-as-needed
> > down on gtest's line, so that the "wrapping" is more obvious. Or maybe 
> > even use some creative indentation like:
> > # ...
> > if (LINUX)
> > set(GOOGLEMOCK_LIBRARIES 
> > gmock
> > -Wl,--no-as-needed # VWR-24366: gmock is underlinked, it needs 
> > gtest.
> > gtest
> > -Wl,--as-needed) # continue to use "as-needed" for other libs
> > elseif(WINDOWS)
> > # ...
> 
> Aleric Inglewood wrote:
> I don't think you can add comments like that in the middle of a SET( ... 
> # ... ) ? Never tried it though.
> If GOOGLEMOCK_LIBRARIES would end up like "gmock -Wl,--no-as-needed # 
> VWR-24366: gmock is underlinked, it needs gtest. gtest -Wl,--as-needed" then 
> that definitely will break things ;)
> 
> @merov: https://codereview.secondlife.com/r/95/ is still open and present 
> on this reviewboard. It's about this same patch?
>

Also, I think that what merov is looking for is someone who builds on linux 
(standalone or not) to actually test this patch... since building is the only 
thing that is influenced. Since a TC build doesn't set CMAKE_EXE_LINKER_FLAGS, 
you probably should be someone who builds standalone and sets 
CMAKE_EXE_LINKER_FLAGS. Alternatively, you can fake setting it (just add 
"-Lhello_world" for example) and confirm that survives with patch. A positive 
test of the patch would need you to install any needed libs in a non-default 
path that actually need that -Lpath and check that the viewer doesn't link 
anymore without the patch, but does with. In all cases makes sure you have 
LL_TESTS on. I'll add a full test plan to 
http://jira.secondlife.com/browse/STORM-981


- Aleric


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/147/#review350
---


On Feb. 9, 2011, 10:20 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/147/
> ---
> 
> (Updated Feb. 9, 2011, 10:20 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> From Aleric's patch:
> 
> Setting CMAKE_EXE_LINKER_FLAGS to "" because the tests "need" that is pretty 
> hard measure.  Not only is it not necessary to do so, it also changes how the 
> viewer is linked depending on a whether or not the tests are compiled and 
> that is not good.
> 
> The reason that this was needed is that libgmock is underlinked (see 
> http://wiki.mandriva.com/en/Underlinking), which is not compatible with 
> -Wl,--as-needed that is being used on linux. libgmock.so.0 needs a symbol 
> that is defined in libgtest.so.o, but -lgtest was not passed to the linker 
> when creating libgmock.so.0:
> 
> Underlinked (no libgtest.so.o):
> $ objdump -p /usr/lib/libgmock.so.0 | grep NEEDED
>   NEEDED   libstdc++.so.6
>   NEEDED   libm.so.6
>   NEEDED   libc.so.6
>   NEEDED   libgcc_s.so.1
> 
> The solution is to wrap between -Wl,--no-as-needed -lgtest -Wl,--as-needed 
> causing it to be added again. This is only needed on linux, since that the 
> only platform that we use -Wl,--as-needed on. Moreover, we can just set 
> GOOGLEMOCK_LIBRARIES to "gmock -Wl,--no-as-needed gtest -Wl,--as-needed" 
> since that is only passed to TARGET_LINK_LIBRARIES which only adds -l in 
> front of 'things' that don't start with '-', to allow you do pass special 
> flags like this.
> 
> 
> This addresses bug STORM-981.
> http://jira.secondlife.com/browse/STORM-981
> 
> 
> Diffs
> -
> 
>   doc/contributions.txt 27dae7b01a81 
>   indra/cmake/GoogleMock.cmake 27dae7b01a81 
>   indra/cmake/LLAddBuildTest.cmake 27dae7b01a81 
> 
> Diff: http://codereview.secondlife.com/r/147/diff
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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

[opensource-dev] Daily Scrum Summary - Thursday, February 10

2011-02-10 Thread Anya Kanevsky

 Thursday, February 10
[edit
]General Notes
--

   - MMOTD: Merov
   - SNOW project is archived. Thank you to all who made it happen!

[edit
]Team Status
--
 
[edit
]Merov Linden
--

*PAST*

   - MM: Pushed 6 pending JIRAs + STORM-986. Mac freeze issue didn't repro.
   Seemed to be related to cache issues.
   - STORM-937 : Path to python in /scripts/*.py file inconsistent : updated
   wiki again, post new RB, tested build on TC but fails on Windows (build.sh
   issue)
   - STORM-981: Linux link issue if -DLL_TESTS:BOOL=ON : Posted patch and
   RB, builds on TC, moved to review.
   - STORM-987 : llimage_libtest : created that task that needs to happen
   before STORM-746. Created first skeleton for those (cmake, etc...).
   - Attended "Open Development User Group": triaged OPEN issues

*FUTURE*

   - MM
   - STORM-937 : Fix the build.sh issue
   - STORM-987 : llimage_libtest: Post first implementation

*IMPEDIMENTS*

   - None

[edit
]Oz Linden
--

*PAST*

   - Open Development User Group meeting
  - Great triage of build issues
   - Much work on rolling out User Groups
   - Some not very successful work on open-4

*FUTURE*

   - VWR issue triage
   - Create wiki page on building viewer w/ autobuild
  - Associated environment setup pages

*IMPEDIMENTS*

   - none

[edit
]Q Linden
--

*PAST*

   - documentation / blog
   - internal meetings

*FUTURE*

   - release
   - triage

*IMPEDIMENTS*

   - none

[edit
]Wolf Linden
--

*PAST*

   - went through all jira in needs-design queue and commented

*FUTURE*

   - STORM-951

*IMPEDIMENTS*

   - none

[edit
]Grumpity ProductEngine
--

*PAST*

   - jira
   - ooo

*FUTURE*

   - STORM cleanup in jira
   - QA updates
   - triage
   - request jira modifications
   - look through wolf’s comments

*IMPEDIMENTS*

   - none

[edit
]Paul Productengine
--

*PAST*

   - BUG STORM-357 (Gestures button is in the pressed state after
   drag-n-drop but gestures list isn't visible)
  - Fixed and sent for review

*FUTURE*

   - BUG STORM-533 (User is unable to save snapshot after pressing "More"
   and "Less" (without any changes)

*IMPEDIMENTS*

   - none

[edit
]Seth Productengine
--

*PAST*

   - BUG (STORM-833) "i" button is overlaid with Group Members name
  - Fixed. Posted fix for review.
   - Code review

*FUTURE*

   - BUG (STORM-939) Opening multiple inventory floaters causes viewer to
   lag or even crash
  - Estimated: 10-12 hours.

*IMPEDIMENTS*

   - none

[edit
]Vadim Productengine
--

   - OOO - vacation

[edit
]Andrey ProductEngine
--

*PAST*

   - verified 18 issues
   - continued regression testing of v-d branch:
  - My Landmarks Panel (100%)
  - Teleport History Panel (100%)
  - Alerts & Notifications (~50%)
  - Busy mode (100%)
  - SideBar Basics (100%)
  - Chiclets and Chiclet Windows basics - 100%
  - Alerts & Notifications - 40%
  - Home Panel - 100%
  - Nearby People Panel 100%
  - Groups Panel 100%
  - Group Profile Panel 100%
  - Recent People Panel 100%

*FUTURE*

   - continue regression testing
   - update Build tools TP

*IMPEDIMENTS*

   - none

[edit
]Jonathan Yap
--

*PAST*

   - STORM-976 (Object muted by name still displays notification message
   when clicked)
  - Depending on how STORM-951 is fixed nothing may need to be done to
  close this jira.
   - STORM-979 (selection outline from inventory bottom panel buttons is cut
   at the left side when clicked.)

*FUTURE*

   - STORM - 979

*IMPEDIMENTS*

   - none

[edit<

Re: [opensource-dev] Review Request: Use consistent path for all *.py scripts

2011-02-10 Thread Oz Linden


---
This is an automatically generated e-mail. To reply, visit:
http://codereview.secondlife.com/r/129/#review353
---

Ship it!


Those changes look good.

For future reference, there's no reason to add date ranges in Copyright headers 
- the only year needed is the year of first publication.  Adding additional 
years is not needed and serves no purpose.


- Oz


On Feb. 9, 2011, 3:33 p.m., Merov Linden wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> http://codereview.secondlife.com/r/129/
> ---
> 
> (Updated Feb. 9, 2011, 3:33 p.m.)
> 
> 
> Review request for Viewer.
> 
> 
> Summary
> ---
> 
> Simple consistency change, using "#!/usr/bin/python" in all python script.
> 
> 
> This addresses bug STORM-937.
> http://jira.secondlife.com/browse/STORM-937
> 
> 
> Diffs
> -
> 
>   indra/cmake/run_build_test.py 0ca239e469b3 
>   indra/copy_win_scripts/start-client.py 0ca239e469b3 
>   indra/develop.py 0ca239e469b3 
>   indra/lib/python/indra/util/llperformance.py 0ca239e469b3 
>   indra/lib/python/indra/util/simperf_proc_interface.py 0ca239e469b3 
>   indra/lib/python/indra/util/test_win32_manifest.py 0ca239e469b3 
>   indra/llmessage/tests/test_llsdmessage_peer.py 0ca239e469b3 
>   indra/llmessage/tests/testrunner.py 0ca239e469b3 
>   indra/newview/generate_breakpad_symbols.py 0ca239e469b3 
>   indra/newview/tests/test_llxmlrpc_peer.py 0ca239e469b3 
>   indra/newview/viewer_manifest.py 0ca239e469b3 
>   indra/test/test_llmanifest.py 0ca239e469b3 
>   scripts/build_version.py 0ca239e469b3 
>   scripts/md5check.py 0ca239e469b3 
>   scripts/setup-path.py 0ca239e469b3 
>   scripts/template_verifier.py 0ca239e469b3 
>   scripts/update_version_files.py 0ca239e469b3 
> 
> Diff: http://codereview.secondlife.com/r/129/diff
> 
> 
> Testing
> ---
> 
> Pulled into a test repo and build successfully on all platforms on TC so I 
> guess no bad surprise here.
> 
> 
> Thanks,
> 
> Merov
> 
>

___
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

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

Re: [opensource-dev] Review Request: CMAKE_EXE_LINKER_FLAGS not honored when linking the viewer binary if -DLL_TESTS:BOOL=ON

[opensource-dev] Daily Scrum Summary - Thursday, February 10

Re: [opensource-dev] Review Request: Use consistent path for all *.py scripts

5 matches

Site Navigation

Mail list logo

Footer information