STORM-1320 Create a 3p-libndofdev-linux repo based on version 0.3 of Jan Ciger's linux libndofdev.


Submitter:	Log Linden	Reviewers
Branch:		Groups:	viewer
Bugs:	OPEN-21, STORM-1320, STORM-312	People:	oz.linden, boroondas.gupte, Sythos
Change Number:	None	Repository:	viewer-development

Description:

Checked in version 0.3 of Jan Ciger's libndofdev drop-in replacement for linux.
* Added cmake build configuration.
* Added autobuild package configuration.
* Created libndofdev.txt license file from ndofdev.c file header.
* Added README to explain that this is only for use in the linux viewer.

BUGFIXES:
* OPEN-21 STORM-312 This version of libndofdev supports kernel versions >= 2.6.33.

When reviewing, please provide extra scrutiny to autobuild.xml and CMakeLists.txt, since those are the files I actually edited.

Testing Done:

This built successfully on TeamCity and the packaged library worked correctly when I extracted it into the packages directory of the viewer build tree ( build-linux-i686/packages ).  My spacenavigator, which hasn't worked in six months, started working with the new build.

Expand All

Tested: I can produce a package with:

autobuild install
autobuild build
autobuild package

Haven't tested the resulting package, but it's content looks reasonable.

autobuild.xml (Diff revision 1)

I guess it's not a problem that the paths in the package will collide with the paths of the non-linux libndofdev package, or is it?

(If they were built from the same source, they'd collide too, wouldn't they?)

Log Linden June 17, 2011, 6:39 a.m. (June 17, 2011, 6:39 a.m.)

The .a file in /lib has to collide anyway to work right.  Autobuild install shouldn't install more than one of the packages.

libndofdev/CMakeLists.txt (Diff revision 1)

Jan Ciger's libndofdev comes with a Makefile. Why not use that?

This lib is linux-specific, so we don't need a cross-platform build configuration for it.

Log Linden June 17, 2011, 6:58 a.m. (June 17, 2011, 6:58 a.m.)

I would have needed to modify Jan's makefile to fit the autobuild package layout. I also used cmake because the other libndofdev library build also uses it.

libndofdev/CMakeLists.txt (Diff revision 1)

If these flags are added unconditionally, how would one do a 64-bit build? Maybe check for WORD_SIZE, like the viewer build does.

Log Linden June 17, 2011, 8:19 a.m. (June 17, 2011, 8:19 a.m.)

I see m32 hard coded in several other libraries, including 3p-google-mock

libndofdev/CMakeLists.txt (Diff revision 1)

Might be worth mentioning the non-linux libndofdev (and where to find it) in the error message.

Jonathan Yap June 16, 2011, 5:43 p.m. (June 16, 2011, 5:43 p.m.)

On windows I already get 4 lines such as: 
package pcre has no installation information configured for platform windows
which is plenty of information for a package I have no need of; let's not put too much unnecessary text on the screen.

Log Linden June 17, 2011, 6:35 a.m. (June 17, 2011, 6:35 a.m.)

Those messages are only be displayed during build configuration, and only if you try to build the package on windows or mac.  They are there to alleviate confusion because we have two libndofdev packages, depending on the platform. Telling where to find the other library seems like a good idea.

Diff
- added Diff r2

Description:

I incorporated the feedback I got here into a new revision.

* Got rid of the hardcoded -m32 in CMakeLists.txt
* Also got rid of hard coded cmake build type
* Corrected the static library file location to lib/release
* Added the new /lib/release folder to the autobuild manifest and removed /lib
* Added the location of the other libndofdev library to the cmake warnings
* Added a BuildParam to make the built package get uploaded to the public s3

The repo is also now up on bitbucket: http://bitbucket.org/lindenlab/3p-libndofdev-linux
TC Build of this revision: http://automated-builds-secondlife-com.s3.amazonaws.com/hg/repo/3p-libndofdev-linux/rev/233137/arch/Linux/installer/libndofdev-0.3-linux-20110617.tar.bz2
hash: 9bf7a96c1d2fadb180fda91740c945c6

I'll put up the viewer changeset shortly as a separate diff.

My remaining comments are mere bike-shedding, thus giving "Ship it!".

libndofdev/CMakeLists.txt (Diff revisions 1 - 2)

Thanks for making this a bit future-proofer. :-)

ADD_DEFINITION actually adds definitions, keeping previously added ones, doesn't it?

So we could take the '-pipe' out of the conditional code:

    if (WORD_SIZE EQUAL 32)
        ADD_DEFINITIONS("-m32")
    elseif (WORD_SIZE EQUAL 64)
        ADD_DEFINITIONS("-m64")
    endif (WORD_SIZE EQUAL 32)

    ADD_DEFINITIONS("-pipe")


Dunno if that's better though ... if in doubt, keep your version.




(I also thought about replacing the whole block by

    ADD_DEFINITIONS("-m${WORD_SIZE} -pipe")

but I guess an if-elseif is clearer.)

libndofdev/CMakeLists.txt (Diff revisions 1 - 2)

Good message texts! Though, end the second sentence with a period rather than arbitrary whitespace.

You have a pending review.

Other reviews

Your comment

Review Board 1.6.11

This change has been marked as submitted.