Code review comment for lp:~michihenning/unity-scopes-api/single-tree

Thanks for the detailed comments Steve, I appreciate it!

> Why are you hard-coding an soversion in a separate file for vivid? This doesn't look right to me. This implies that if in a later version the ABI changes, the wily package will get an soname change but the vivid package will keep the same soname despite no longer being compatible.

Yes, fixed. Everything is derived now, with additional sanity checks, so it's not possible to set an incorrect version for Vivid.

>> +On Vivid only, we maintain symbols files, using the gcc 4.9 ABI names.
>> +On Wily, there is no symbols file because we don't care whether we
>> +use the gcc 4.9 or 5.x ABI. (The code is the same either way.) Instead,
>> +we use a shlibs file.
>
> Dropping the symbols file is only acceptable if another method is used to check the ABI correctness. I understand that abi-compliance-checker integration is forthcoming.

Yes, that's next on my list. Adding a separate symbols file for Wily wouldn't buy us anything anyway, because the same code is compiled for both Vivid and Wily. So, the Wily symbols file would be different (due to the different mangled name for things using std::string and std::list, and differences in inlining), but otherwise would not detect anything that isn't detected during the check of the Vivid symbols file.

My next project is to figure out how to make abi-compliance-checker usable and to integrate it into the build/test such that it does its magic automatically. Once that works, we'll have a much better assurance for the ABI, and we can finally get rid of the symbols file altogether.

> Critically, these requirements are not going to be apparent to an Ubuntu developer reviewing a packaging diff. Mechanically converting the various version forms (even if that means embedding some magic constants and opaque math in the CMakeLists.txt) from a single source would be a better safeguard against inadvertently introducing an ABI change without correctly updating the versions and package names.

Agreed. We now have two files for each library (one for Vivid and one for Wily) that contains the version number. Everything else for cmake and debian is derived from that, so things are guaranteed to be consistent.

>> + the debian files are no generated from the debian/rules
>
> s/no/now/

Thanks for spotting this, fixed.

>> +sed "s/@UNITY_SCOPES_SOVERSION@/${soversion}/" "${dir}"/control.in \
>> +| sed "s/@UNITY_SCOPES_QT_SOVERSION@/${qt_soversion}/" >>"${dir}"/control
>
> can also be done with a single sed invocation using multiple '-e' arguments

Yes, done. I also simplified things in a number of other places.

>> +cat "${dir}"/libunity-scopes.install.in >>"${dir}"/libunity-scopes${soversion}.install
>
> Not a regression, but best practice would be to include the soname of the library in the .install file so that a (properly declared) upstream ABI change doesn't result in a misbuilt package with the wrong contents for its name.

I'm not totally sure what you mean here. I don't think that substituting the version number explicitly into these files would make things any safer, or am I missing something? The way I see it, with everything derived from a single version that is used everywhere, all the install locations, pkgconfig file, etc. are guaranteed to get the correct version during the build. Or am I missing the point entirely here?

>> +usr/include/unity-scopes-@SOVERSION@/unity-scopes.h
>
> This looks like a regression, not sure how these @SOVERSION@ tags ended up in the generated .install file.

Yes, thanks. that file got left behind when I was hacking around. I've deleted it.

>> === added file 'debian/libunity-scopes-doc.lintian-overrides'
>> --- debian/libunity-scopes-doc.lintian-overrides 1970-01-01 00:00:00 +0000
>> +++ debian/libunity-scopes-doc.lintian-overrides 2015-08-25 00:42:52 +0000
>> @@ -0,0 +1,3 @@
>> +# Nothing we can do about this because, depending on the installed version
>> +# of jquery.js, the tree widget breaks.
>> +libunity-scopes-doc: embedded-javascript-library
>
> Please drop this change. It is unrelated to the rest of the packaging changes; it is also incorrect to override a lintian error except in cases where the lintian error itself is incorrect.

I know it's unrelated, but I was checking the output from by bzr bd builds and picked this up as I went. Debian is indeed wrong here. Quite a lot of people have stumbled across this. The issue is that the jquery.js version that is expected by doxygen may not match the installed version. So, using the installed version can lead to the tree widget breaking. The only option is to use the jquery.js that is supplied by doxygen and, hence, the warning is bogus.

>> === added file 'debian/libunity-scopes.symbols.in'
>
> It seems a number of these files have been renamed to '.in' by deleting the old and adding a new file under a new name. This might be quite a bit more reviewable if you were to use 'bzr mv'.

It was a lot easier for me to create the input files while I still had the original files in the tree. My apologies for the noise.

>> === modified file 'debian/rules'
>> --- debian/rules 2015-08-21 11:32:35 +0000
>> +++ debian/rules 2015-08-25 00:42:52 +0000
>> @@ -22,3 +22,7 @@
>>
>> override_dh_click:
>> dh_click --name scope
>> +
>> +override_dh_auto_clean:
>> + /bin/sh `pwd`/debian/gen-debian-files `pwd`
>> + dh_auto_clean
>
> It would be more idiomatic to generate debian/control in override_dh_auto_clean, but generate the other autogenerated files in override_dh_auto_build (and remove them again in the clean target)

Is this really necessary? I'm worried that things get even more complicated if there are two scripts that run at different times. What is there now works, so I'd be inclined to not make this change.

>> +libunity-scopes @UNITY_SCOPES_SOVERSION@ libunity-scopes@UNITY_SCOPES_SOVERSION@ (>= @UNITY_SCOPES_FULL_VERSION@)
>> +libunity-scopes-qt @UNITY_SCOPES_QT_SOVERSION@ libunity-scopes-qt@UNITY_SCOPES_QT_SOVERSION@ (>= @UNITY_SCOPES_QT_FULL_VERSION@)
>
> This changes the generated dependencies from (>= 1.0.0) to (>= 1.0.1) (etc.) - is that the intended effect?

Thanks for catching this! No, that was wrong. The dependency for the shlibs file needs to be the last time the ABI was changed which, by definition, is whenever the major or minor version changes. I'm no generating ">= <major>.<minor>" here.

>> + virtual void foo() {}
>
> test code, I guess, that needs removed?

Indeed, thanks! :-)