Merge lp:~mardy/ubuntuone-credentials/library-symbols into lp:ubuntuone-credentials

[Alberto Mardegan]

Be more explicit about which headers are installed.
Move the symbol export map to LINK_FLAGS on the target.

Be explicit about exported classes and public headers

Set hidden visibility on all internal symbols (none, currently) and explicitly mark as exported those classes which should be public.
Similarly, don't install all headers (we might be adding some internal ones in the future) but explicitly list those which should be installed.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:237
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/143/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/20
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/20
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/20/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/20

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/143/rebuild

review: Approve (continuous-integration)

[dobey (dobey)]

I am not fond of the conditional exporting of symbols and the addition of a #define for that. Why do we need that? We are already controlling symbol exports through the version script.

review: Needs Information

[Alberto Mardegan (mardy)]

I find it easier, when the information is in the class declaration itself. At least, I though that this is the explanation why some obviously internal class (like UbuntuOne::Network) is exported in the first place: because the version symbols is kind of "hidden".

But if you prefer to continue with the version script, that's fine, I'll drop the relevant changes.

And did you ever think about using PIMPL? Right now we are exporting a lot of private methods and member variables, which makes it rather difficult to modify the library without breaking ABI.

[Alberto Mardegan (mardy)]

BTW, the line changing the way the --version-script is passed to the linker is the fix for the bug you reported here:
https://bugs.launchpad.net/ubuntu/+source/cmake/+bug/1203786

[dobey (dobey)]

> I find it easier, when the information is in the class declaration itself. At
> least, I though that this is the explanation why some obviously internal class
> (like UbuntuOne::Network) is exported in the first place: because the version
> symbols is kind of "hidden".
>
> But if you prefer to continue with the version script, that's fine, I'll drop
> the relevant changes.
>
> And did you ever think about using PIMPL? Right now we are exporting a lot of
> private methods and member variables, which makes it rather difficult to
> modify the library without breaking ABI.

Perhaps explicit export flags in declarations is easier, but certainly not when it is conditional. "Internal" bits are leaked because the code was not well designed for ABI/API exposure to start with. That's what happens when you make developers who primarily write Python, suddenly have to write a Qt/C++ API though.

Yes, PIMPL makes sense, but due to the aforementioned issues it would indeed break the API/ABI, and to get it right, would basically be a rewrite. If we were going to do a rewrite, I think I would go about redoing the API entirely anyway, so just trying to make the existing stuff use PIMPL won't help much.

Perhaps when we finish up getting the signon plug-in working and we use it everywhere, we can then change the components which use the U1 account to use the more traditional/standard methods of getting the data from online-accounts, and possibly get rid of the library.

So, I'm not quite sure what to do with this branch yet. But as I am definitely averse to the idea of conditional export flags adding a #define for that, there isn't a specific bug being fixed by these changes, and they don't seem to be required for the signon-plugin branches, I'm inclined to reject this one, in favor of further discussion and examination of what we can gain from the signon-plugin work.

[lp:~mardy/ubuntuone-credentials/library-symbols]

238. By Alberto Mardegan on 2016-04-21

Partial revert

[Alberto Mardegan (mardy)]

OK, I removed all the conditional exports.

Now the MP consists only of the header files install list (which we'll need, as I'm later going to add a couple of internal classes to the library) and the version-script move so that it doesn't get applied to the other targets of the project (otherwise the signon plugin cannot be linked, as its external symbols get stripped off).

I agree with you, that we should aim at not changing the library API and ABI if possible, and eventually have clients authenticate only via the OA APIs.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:238
http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-ci/145/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-amd64-ci/22
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/22
        deb: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-armhf-ci/22/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntuone-credentials-wily-i386-ci/22

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntuone-credentials-ci/145/rebuild

review: Approve (continuous-integration)

[dobey (dobey)]

review: Approve

=== modified file 'libubuntuoneauth/CMakeLists.txt'
--- libubuntuoneauth/CMakeLists.txt	2013-11-22 19:17:04 +0000
+++ libubuntuoneauth/CMakeLists.txt	2016-04-21 09:28:24 +0000
@@ -16,7 +16,17 @@
 # The sources for building the library
 FILE (GLOB SOURCES *.cpp)
 # HEADERS only includes the public headers for installation.
-FILE (GLOB HEADERS *.h)
+FILE (GLOB HEADERS
+  errormessages.h
+  identityprovider.h
+  keyring.h
+  logging.h
+  network.h
+  requests.h
+  responses.h
+  ssoservice.h
+  token.h
+)
 
 pkg_check_modules(OAUTH REQUIRED oauth)
 add_definitions(${OAUTH_CFLAGS} ${OAUTH_CFLAGS_OTHER})
@@ -29,12 +39,12 @@
 target_link_libraries (${AUTH_LIB_NAME}
   ${LIBSIGNON_LDFLAGS}
   ${OAUTH_LDFLAGS}
-  -Wl,--version-script -Wl,${CMAKE_CURRENT_SOURCE_DIR}/libubuntuoneauth.symbols
 )
 
 SET_TARGET_PROPERTIES(${AUTH_LIB_NAME} PROPERTIES
   VERSION ${AUTH_LIB_VERSION}
   SOVERSION ${AUTH_LIB_SOVERSION}
+  LINK_FLAGS "-Wl,--version-script -Wl,\"${CMAKE_CURRENT_SOURCE_DIR}/libubuntuoneauth.symbols\""
 )
 
 INSTALL (
@@ -64,4 +74,4 @@
 )
 
 add_subdirectory(tests)
-add_subdirectory(examples)
\ No newline at end of file
+add_subdirectory(examples)