Merge lp:~michihenning/unity-api/rename-config into lp:unity-api

[Michi Henning]

Renamed config.h -> DllExport.h. Removed Version.h.cmake, which is was moldy oldie, now called Version.h.in.

Renamed config.h -> DllExport.h. Removed Version.h.cmake, which is was moldy oldie, now called Version.h.in.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:49
http://jenkins.qa.ubuntu.com/job/unity-api-ci/42/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/42
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/42/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-i386-ci/42

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/42/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

Uhm, DLL's as in Dynamically Linked Libraries is a Microsoft Windows feature. Seems like a bad name.

[Michi Henning (michihenning)]

Hmmm... I was under the impression that "dynamically linked library" was a generic term, rather than a Windows-specific one (although I may be mistaken here).

If you feel strongly about it, I can rename it again. ShlibExport.h acceptable? If not, please suggest a name.

[Ted Gould (ted)]

On Thu, 2013-05-30 at 04:03 +0000, Michi Henning wrote:

> Hmmm... I was under the impression that "dynamically linked library"
> was a generic term, rather than a Windows-specific one (although I may
> be mistaken here).

The Wikipedia entry starts with:

Dynamic-link library (also written unhyphenated), or DLL, is Microsoft's
implementation of the shared library concept in the Microsoft Windows
and OS�2 operating systems.

http:��en.wikipedia.org�wiki�Dynamic-link_library

> If you feel strongly about it, I can rename it again. ShlibExport.h
> acceptable? If not, please suggest a name.

I'm not sure what the file actually does, so I don't have a good idea
for a name. But considering every header file in �usr�include
represents something exported by a shared library, I don't think that's
a very useful name. Honestly, even looking at the file, I have no clue
what it does or why an application author needs that. Seems like an
internal file to me. I don't know why an application would want to
change the visibility of symbols by setting a define.

Also, as an aside, it seems that file's header says it's GNU GPL. Is
that on purpose or should it be GNU LGPL?

[Michi Henning (michihenning)]

Not sure whether Wikipedia is the best authority here. I can find quite a few web pages that talk about "dynamically linked shared libraries" in a Linux context.

Anyway, I can call it ShlibExport.h instead.

The whole thing is there to limit public symbols for the .so to only those are part of the public API. The entire thing is inspired by this article: http://gcc.gnu.org/wiki/Visibility

The library is compiled with -fvisibility=hidden so, by default, no symbols are exported. To turn on exporting for the parts of the API that are public, I do:

#include <unity/ShlibExport.h>

class UNITY_API MyClass { ... };

This makes MyClass visible in the .so as a public symbol.

When compiling the library, we compile with -DUNITY_DLL_EXPORTS, so the UNITY_API macro expands to

__attribute__((visibility("default")))

This overrides the -fvisibility=hidden command-line argument.

When the code is compiled without -DUNITY_DLL_EXPORTS, which is what happens when the user compiles with our headers, the UNITY_API macro expands to nothing. (On Windows, the macro would be defined to generate the requisite dllimport and dllexport declspecs.)

Removing the file altogether would be possible, but only at the cost of explicitly redefining the same thing over and over again in each public API header, which is worse, IMO.

The file is installed in /usr/include/unity. We can't not install it because then the public header files would no longer compile.

We could avoid installing it if we were to include it like this in our headers everywhere:

#ifndef UNITY_DLL_EXPORTS
#include <unity/internal/ShlibExport.h>
#endif

But that's a pain too...

[Michi Henning (michihenning)]

> Also, as an aside, it seems that file's header says it's GNU GPL. Is
> that on purpose or should it be GNU LGPL?

I have no idea what it is supposed to be. I picked up the copyright notice some time last year from somewhere on the company Wiki.

Personally, I'm having a problem with including such a copyright message in every source and header file because, if it ever needs to change, fixing it is a massive pain. It would be nicer to have something that includes the license by reference.

At any rate, if you know someone who can tell me definitely what text we should put into every source file, I'll happily put it there. But I'd rather go through the exercise only once.

[Jussi Pakkanen (jpakkane)]

AFAIK "dll" is a windowsism and the corresponding unix term is "shared library".

Semantic fine tuning aside, since this header is about symbol visibility in Unity, how about calling it "unity-visibility.h".

[Michi Henning (michihenning)]

I would suggest SymbolExport.h. Can everyone live with that? No need for unity prefix--the header is in a unity directory already.

[Jussi Pakkanen (jpakkane)]

No file of that name exists in any Ubuntu package, so fine by me.

review: Approve

[lp:~michihenning/unity-api/rename-config]

50. By Michi Henning on 2013-05-30

Renamed DllExport.h to SymbolExport.h

[Ted Gould (ted)]

On Thu, 2013-05-30 at 04:37 +0000, Michi Henning wrote:

> This overrides the -fvisibility=hidden command-line argument.

What we're doing in the HUD header files there is this pragma statment:

  #pragma GCC visibility push(default)

At the start and then at the end:

  #pragma GCC visibility pop

Which makes it so that everything in between those two lines is visible.
And it is independent of the compiler parameters. Makes it so that you
don't have to do anything on a per-object basis.

[Ted Gould (ted)]

On Thu, 2013-05-30 at 04:39 +0000, Michi Henning wrote:

> Personally, I'm having a problem with including such a copyright
> message in every source and header file because, if it ever needs to
> change, fixing it is a massive pain. It would be nicer to have
> something that includes the license by reference.

Short story, would be nice, but probably not going to happen. Gotten in
that discussion more than once. :-)

> At any rate, if you know someone who can tell me definitely what text
> we should put into every source file, I'll happily put it there. But
> I'd rather go through the exercise only once.

I can't tell you definitely. By default the Canonical policy is that
all code is GPLv3. Generally, there is an exception granted for
libraries that are expected to be used by external programs that they
can be LGPLv2�LGPLv3. But, that exception needs to be granted. If a
library is GPL it means that any program linking to that library needs
to be GPL as well.

[Michi Henning (michihenning)]

> What we're doing in the HUD header files there is this pragma statment:

Thanks for that! I'll have a look at this. That might be a better option than the header file.

I'll have a tinker with this tomorrow.

[Michi Henning (michihenning)]

> I can't tell you definitely. By default the Canonical policy is that
> all code is GPLv3.

Well, I guess it's OK for now, seeing that the current copyright is GPLv3.

If someone wants to change it LGPLv3 later, they can let us know, and we can change all the copyright message. I'm glad McMahon wrote sed a long time ago...

[Jussi Pakkanen (jpakkane)]

Both approaches work and give the same results so this is an issue of taste. The first approach is more explicit while the #pragma one is simpler if you want to expose a bunch of stuff all at once (though it is basically a global variable with all the downsides that come with it).

For what it's worth, GCC wiki recommends the former approach:

http://gcc.gnu.org/wiki/Visibility

[Michi Henning (michihenning)]

> Both approaches work and give the same results so this is an issue of taste.
> The first approach is more explicit while the #pragma one is simpler if you
> want to expose a bunch of stuff all at once (though it is basically a global
> variable with all the downsides that come with it).
>
> For what it's worth, GCC wiki recommends the former approach:
>
> http://gcc.gnu.org/wiki/Visibility

Hmmm... I'm sitting on the fence right now with this one. With the strict separation into public header files and private header files (in an internal subdirectory), we could blindly go and just add the #pragma push/pop to every header file that's public, which is attractive.

I agree with you, either approach achieves the same thing. But I've never been fond of the extra clutter that is added by the macro:

    class UNITY_API MyClass

is not as easy to read as

    class MyClass

Of course, the counter-argument is that, by having the macro, we make it explicit at each particular definition that this thing is meant for public consumption, instead of having a global push/pop pragma that brackets everything in between. With the pragmas, if I add something in the middle of a header that isn't meant for public consumption, I end up exporting it when it wasn't meant to be exported.

Overall, I'm leaning slightly towards the header approach with the explicit macro, by a slim margin. Talk about splitting hairs...

Now, getting down to reality, I don't think that either approach would significantly compromise the quality of our software...

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:50
http://jenkins.qa.ubuntu.com/job/unity-api-ci/43/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/43
        deb: http://jenkins.qa.ubuntu.com/job/unity-api-raring-armhf-ci/43/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-api-raring-i386-ci/43

Click here to trigger a rebuild:
http://s-jenkins:8080/job/unity-api-ci/43/rebuild

review: Approve (continuous-integration)

[Ted Gould (ted)]

On Thu, 2013-05-30 at 13:45 +0000, Michi Henning wrote:

> Of course, the counter-argument is that, by having the macro, we make
> it explicit at each particular definition that this thing is meant for
> public consumption, instead of having a global push�pop pragma that
> brackets everything in between. With the pragmas, if I add something
> in the middle of a header that isn't meant for public consumption, I
> end up exporting it when it wasn't meant to be exported.

My thought is that if you put something in a header file
in �usr�include, and you don't expect people to use it, you're a
jerk :-) Don't do that!

Which is why I prefer to say "this file is public" and "this file is
not" instead of doing it on a class by class basis.

[Michi Henning (michihenning)]

Let's run with the current scheme. It does what we need.

=== modified file 'include/unity/Exception.h'
--- include/unity/Exception.h	2013-05-13 11:39:49 +0000
+++ include/unity/Exception.h	2013-05-30 12:25:31 +0000
@@ -19,7 +19,7 @@
 #ifndef UNITY_EXCEPTION_H
 #define UNITY_EXCEPTION_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <exception>
 #include <string>

=== renamed file 'include/unity/config.h' => 'include/unity/SymbolExport.h'
--- include/unity/config.h	2013-05-13 11:39:49 +0000
+++ include/unity/SymbolExport.h	2013-05-30 12:25:31 +0000
@@ -16,8 +16,8 @@
  * Authored by: Michi Henning <[e-mail]>
  */
 
-#ifndef UNITY_CONFIG_H
-#define UNITY_CONFIG_H
+#ifndef SYMBOL_EXPORT_H
+#define SYMBOL_EXPORT_H
 
 #define UNITY_HELPER_DLL_EXPORT __attribute__ ((visibility ("default")))
 

=== removed file 'include/unity/api/Version.h.cmake'
--- include/unity/api/Version.h.cmake	2013-04-03 02:01:36 +0000
+++ include/unity/api/Version.h.cmake	1970-01-01 00:00:00 +0000
@@ -1,101 +0,0 @@
-//
-// DO NOT EDIT Version.h (this file)! It is generated from Version.h.cmake.
-//
-
-/*
- * Copyright (C) 2013 Canonical Ltd
- *
- * This program is free software: you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 3 as
- * published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License
- * along with this program.  If not, see <http://www.gnu.org/licenses/>.
- *
- * Authored by: Michi Henning <[e-mail]>
- */
-
-#ifndef UNITY_API_VERSION_H
-#define UNITY_API_VERSION_H
-
-#define UNITY_API_VERSION_MAJOR @UNITY_API_MAJOR@
-#define UNITY_API_VERSION_MINOR @UNITY_API_MINOR@
-#define UNITY_API_VERSION_MICRO @UNITY_API_MICRO@
-
-#define UNITY_API_VERSION_STRING "@UNITY_API_VERSION@"
-
-namespace unity
-{
-
-namespace api
-{
-
-/**
-\brief Class to obtain version information for the Unity API at run time.
-
-Version information is represented as
-<i>&lt;<code>major</code>&gt;</i>.<i>&lt;<code>minor</code>&gt;</i>.<i>&lt;<code>micro</code>&gt;</i>.
-
-Releases that differ in the major or minor version number are binary incompatible.
-
-Releases of the library that differ only in the micro version number are binary compatible with older releases,
-so client code does not need to be recompiled to use the newer library version.
-
-A different minor version is compatible at the API level, that is, it may add new APIs, but does not change existing
-ones. API clients must be recompiled for a new major version.
-
-A different major version indicates incompatible API changes.
-*/
-
-// Version could be a namespace instead of a class, but that requires a lower-case name,
-// which is inconsistent with the remainder of the API.
-
-class Version
-{
-public:
-    /**
-    \brief Returns the major version number of the Unity API library.
-
-    The major version number is also available as the macro <code>UNITY_API_VERSION_MAJOR</code>.
-    */
-    static int major_version();
-
-    /**
-    \brief Returns the minor version number of the Unity API library.
-
-    The minor version number is also available as the macro <code>UNITY_API_VERSION_MINOR</code>.
-    */
-    static int minor_version();
-
-    /**
-    \brief Returns the micro version number of the Unity API library.
-
-    The micro version number is also available as the macro <code>UNITY_API_VERSION_MICRO</code>.
-    */
-    static int micro_version();
-
-    /**
-    \brief Returns the Unity API version as a string in the format
-    <i>&lt;<code>major</code>&gt;</i>.<i>&lt;<code>minor</code>&gt;</i>.<i>&lt;<code>micro</code>&gt;</i>.
-
-    The version string is also available as the macro <code>UNITY_API_VERSION_STRING</code>.
-    */
-    static const char* str();    // Returns "major.minor.micro"
-
-    // TODO: Add methods to report compiler version and compiler flags
-
-private:
-    Version() = delete;
-    ~Version() = delete;
-};
-
-} // namespace api
-
-} // namespace unity
-
-#endif

=== modified file 'include/unity/api/Version.h.in'
--- include/unity/api/Version.h.in	2013-05-16 11:48:42 +0000
+++ include/unity/api/Version.h.in	2013-05-30 12:25:31 +0000
@@ -20,7 +20,7 @@
  * Authored by: Michi Henning <[e-mail]>
  */
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #ifndef UNITY_API_VERSION_H
 #define UNITY_API_VERSION_H

=== modified file 'include/unity/shell/notifications/Enums.h'
--- include/unity/shell/notifications/Enums.h	2013-05-16 07:43:20 +0000
+++ include/unity/shell/notifications/Enums.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef UNITY_SHELL_NOTIFICATIONS_ENUMS_H
 #define UNITY_SHELL_NOTIFICATIONS_ENUMS_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <QtCore/QObject>
 

=== modified file 'include/unity/shell/notifications/ModelInterface.h'
--- include/unity/shell/notifications/ModelInterface.h	2013-05-16 10:24:52 +0000
+++ include/unity/shell/notifications/ModelInterface.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef UNITY_SHELL_NOTIFICATIONS_MODELINTERFACE_H
 #define UNITY_SHELL_NOTIFICATIONS_MODELINTERFACE_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <QtCore/QAbstractListModel>
 

=== modified file 'include/unity/shell/notifications/NotificationInterface.h'
--- include/unity/shell/notifications/NotificationInterface.h	2013-05-16 07:51:36 +0000
+++ include/unity/shell/notifications/NotificationInterface.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef UNITY_SHELL_NOTIFICATIONS_NOTIFICATIONINTERFACE_H
 #define UNITY_SHELL_NOTIFICATIONS_NOTIFICATIONINTERFACE_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <QtCore/QObject>
 

=== modified file 'include/unity/shell/notifications/SourceInterface.h'
--- include/unity/shell/notifications/SourceInterface.h	2013-05-16 10:24:52 +0000
+++ include/unity/shell/notifications/SourceInterface.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef UNITY_SHELL_NOTIFICATIONS_SOURCEINTERFACE_H
 #define UNITY_SHELL_NOTIFICATIONS_SOURCEINTERFACE_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <QtCore/QObject>
 

=== modified file 'include/unity/util/FileIO.h'
--- include/unity/util/FileIO.h	2013-05-13 11:39:49 +0000
+++ include/unity/util/FileIO.h	2013-05-30 12:25:31 +0000
@@ -19,7 +19,7 @@
 #ifndef UNITY_UTIL_FILEIO_H
 #define UNITY_UTIL_FILEIO_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <string>
 #include <vector>

=== modified file 'include/unity/util/NonCopyable.h'
--- include/unity/util/NonCopyable.h	2013-05-13 11:39:49 +0000
+++ include/unity/util/NonCopyable.h	2013-05-30 12:25:31 +0000
@@ -31,7 +31,7 @@
 #ifndef UNITY_UTIL_NONCOPYABLE_H
 #define UNITY_UTIL_NONCOPYABLE_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 namespace unity
 {

=== modified file 'test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockActionModel.h'
--- test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockActionModel.h	2013-05-14 08:12:15 +0000
+++ test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockActionModel.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef MOCKACTIONMODEL_H
 #define MOCKACTIONMODEL_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <QtCore/QAbstractListModel>
 

=== modified file 'test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockModel.h'
--- test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockModel.h	2013-05-14 08:12:15 +0000
+++ test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockModel.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef MOCKMODEL_H
 #define MOCKMODEL_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <unity/shell/notifications/ModelInterface.h>
 

=== modified file 'test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockNotification.h'
--- test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockNotification.h	2013-05-15 13:42:26 +0000
+++ test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockNotification.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef MOCKNOTIFICATION_H
 #define MOCKNOTIFICATION_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <unity/shell/notifications/NotificationInterface.h>
 

=== modified file 'test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockSource.h'
--- test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockSource.h	2013-05-14 08:12:15 +0000
+++ test/qmltest/mocks/plugins/Unity/Notifications/Mocks/MockSource.h	2013-05-30 12:25:31 +0000
@@ -21,7 +21,7 @@
 #ifndef MOCKSOURCE_H
 #define MOCKSOURCE_H
 
-#include <unity/config.h>
+#include <unity/SymbolExport.h>
 
 #include <unity/shell/notifications/SourceInterface.h>