Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libtidy.so.5 has removed symbols between 5.2.0 and 5.6.0, but kept SONAME #743

Closed
oerdnj opened this issue Jul 7, 2018 · 30 comments
Closed

Comments

@oerdnj
Copy link

oerdnj commented Jul 7, 2018

When (exported) symbols are removed from the library it's important for library to bump the SONAME for programs not to break. This hasn't happened and it needs to be done.

I would rather bump the version in a coordinated manner than do that only in Debian.

Excerpts:

+#MISSING: 1:5.6.0-0.1# prvTidyDisplayHTMLTableAlgorithm@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidyNeedsAuthorIntervention@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidyReportAccessWarning@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidyReportError@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidyReportFatal@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidytidyErrorCodeListSize@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidytidyInstalledLanguageListSize@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidytidyLanguageListSize@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# prvTidytidyStringKeyListSize@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetABBR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetALINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetALT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetBGCOLOR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetCHECKED@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetCOLSPAN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetCONTENT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetDATAFLD@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetFOR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetHEIGHT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetHREF@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetHTTP_EQUIV@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetID@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetISMAP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetLANG@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetLANGUAGE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetLONGDESC@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetNAME@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnBLUR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnCLICK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnFOCUS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnKEYDOWN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnKEYPRESS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnKEYUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnMOUSEDOWN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnMOUSEMOVE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnMOUSEOUT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnMOUSEOVER@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetOnMOUSEUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetREL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetROWSPAN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetSELECTED@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetSRC@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetSTYLE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetSUMMARY@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetTARGET@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetTEXT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetTITLE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetTYPE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetUSEMAP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetVALUE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetVLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetWIDTH@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrGetXMLNS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsABBR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsALINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsALT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsBGCOLOR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsCHECKED@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsCOLSPAN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsCONTENT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsDATAFLD@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsFOR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsHEIGHT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsHREF@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsHTTP_EQUIV@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsID@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsISMAP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsLANG@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsLANGUAGE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsLONGDESC@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsNAME@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnBLUR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnCLICK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnFOCUS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnKEYDOWN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnKEYPRESS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnKEYUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnMOUSEDOWN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnMOUSEMOVE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnMOUSEOUT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnMOUSEOVER@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsOnMOUSEUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsProp@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsREL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsROWSPAN@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsSELECTED@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsSRC@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsSTYLE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsSUMMARY@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsTARGET@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsTEXT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsTITLE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsTYPE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsUSEMAP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsVALUE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsVLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsWIDTH@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyAttrIsXMLNS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsA@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsADDRESS@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsAPPLET@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsAREA@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsB@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBASE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBASEFONT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBIG@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBLOCKQUOTE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBODY@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsBR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsCAPTION@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsCENTER@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsCOL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsCOLGROUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDATALIST@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDD@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDIR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDIV@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsDT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsEM@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsEMBED@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsFONT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsFORM@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsFRAME@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsFRAMESET@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH1@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH2@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH3@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH4@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH5@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsH6@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsHEAD@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsHR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsHTML@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsI@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsIFRAME@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsIMG@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsINPUT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsISINDEX@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsLABEL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsLAYER@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsLI@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsLINK@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsLISTING@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsMAP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsMARQUEE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsMENU@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsMETA@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsNOBR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsNOFRAMES@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsNOSCRIPT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsOBJECT@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsOL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsOPTGROUP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsOPTION@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsPARAM@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsPRE@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsU@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsUL@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsWBR@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidyNodeIsXMP@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidySetReportFilter2@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidySetReportFilter3@Base 5.2.0
+#MISSING: 1:5.6.0-0.1# tidySystemLocale@Base 5.2.0

Also if prvTidy means private symbols, you need to use symbol visibility. If the symbol is out there (e.g. exported) it doesn't matter what name you pick: https://gcc.gnu.org/wiki/Visibility (the page says C++, but it should be used for C). In your case it would be easier to use LD Version Script: https://www.gnu.org/software/gnulib/manual/html_node/LD-Version-Scripts.html

@oerdnj
Copy link
Author

oerdnj commented Oct 16, 2018

Since the issue is apparently being ignored here, I am just announcing that the package in Debian will bump the SONAME to libtidy.so.6.

@oerdnj
Copy link
Author

oerdnj commented Oct 16, 2018

Specifically, this is wrong:

    set_target_properties( ${name} PROPERTIES
                                   VERSION   ${LIBTIDY_VERSION}
                                   SOVERSION ${TIDY_MAJOR_VERSION} )

The VERSION and SOVERSION of the library needs to be independent to the version of the sources.

@hosiet
Copy link

hosiet commented Oct 16, 2018

This issue is being tracked and discussed downstream in Debian right here: https://bugs.debian.org/911130

@geoffmcl
Copy link
Contributor

@oerdnj, @hosiet, all, sorry for the delay... I was hoping a unix person, with more understanding of unix shared libraries, and their .so version, would pick this up...

As suggested, the so called missing tidyAttrIs[name], tidyAttrGet[name], and tidyNodeIs[element] functions were marked as deprecated as far back as about 2008, or before, and alternative API functions given... any package still using them abt 10 years later has to be redone... quite unlikely there would be any package breakage otherwise...

Since its beginning libTidy has used a MACRO, TY_(name)(...) to hide non-public functions... just internally used across objects of the library... these are prefixed with prvTidy, making it clear they are not public functions... they, and the macro, are not included in the published tidy.h header... any package using libTidy should not contain these functions, and deserves to break if they do...

As you may know, the MS Windows compiler/linker handles this hiding by not including them in the important import library, .lib, that accompanies the .dll shared library, through the __declspec(dllimport|dllexport) macro, that is TIDY_EXPORT...

So maybe someone with unix understanding should/could do something similar for a gcc compile/link... they are all declared as TY_(name)...

Given this explanation, is it really necessary to bump the SOVERSION to 6?

Anyway, look forward to more feedback...

@oerdnj
Copy link
Author

oerdnj commented Oct 19, 2018

@geoffmcl Unfortunately, yes, it is needed to bump to SOVERSION and decouple the source version with library version. You cannot rely on marking functions as deprecated as people might still be using them.

I am not well accustomed with cmake, but I can probably make PR to do the library versioning properly if you want. And it should be fairly easy to tweak TIDY_EXPORT for GCC/GNU ld, so it declares the requested symbol visibility also on Linux/Unix.

@oerdnj
Copy link
Author

oerdnj commented Oct 19, 2018

and deserves to break if they do...

While the developer in me agrees with you, the package maintainer screams in horror :)

@hosiet
Copy link

hosiet commented Oct 19, 2018

Deprecating a function is easy, but removing it would be really costly and you have to bump the SONAME when doing removal to prevent any and every user from encountering missing symbols (missing functions) when using a newer version of library (of the same SONAME.

So the workflow is clear: feel free to deprecate a function, but whenever you remove one (or change API / break ABI), bump the SONAME immediately.

@hosiet
Copy link

hosiet commented Oct 19, 2018

(BTW: remove function --> soname bump only applies to exported public symbols (functions). If you believe all changes are made on private functions (prvTidy...) then it is not necessary. But you removed tidyAttrIs[name], tidyAttrGet[name], and tidyNodeIs[element] from v5.2 to v5.6 so the soname has to be bumped this time anyway.)

@cmb69
Copy link
Contributor

cmb69 commented Oct 19, 2018

@oerdnj Would bumping the SONAME to libtidy.so.5.6 be an option? I'm afraid that libtidy.so.6 might cause issues, if there'll ever be an libtidy 6.

@hosiet
Copy link

hosiet commented Oct 19, 2018

In fact SONAME can only be an (rising) integer, from 0 to 1, 2, 3, ... On Unix-like systems, the programs that are looking for dynamic libraries will only search for something like libfoobar.so.SONAME where SONAME is an integer.

So your proposal is not valid :-/ It's really okay to make the project version and SONAME different and there's already countless examples all around.

@cmb69
Copy link
Contributor

cmb69 commented Oct 20, 2018

@hosiet Thanks for the explanation!

@oerdnj
Copy link
Author

oerdnj commented Oct 20, 2018

That’s not entirely true, the Debian now has libtidy.so.5deb1 to make room for upstream to use 6 as next. Nevertheless the library soname has to be decoupled from source version. There’s no reason to keep them same.

@geoffmcl
Copy link
Contributor

@hosiet WOW, I am not really the person to be discussing this... The process of creating a libtidy.so link to a libtidy.so.$SOVERSION, which then links to a libtidy.so.${LIBTIDY_VERSION}, which is the actual shared library code, does not apply to Windows native, and while I understand some the history of this, it is just not my cup of tea ;=))

It also seem quite normal for the SOVERSION to be the same as the ${TIDY_MAJOR_VERSION}... yes I can see decoupling them is possible, and experimented with something like -

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 631f0fd..227e1ea 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -405,12 +405,15 @@ if (BUILD_SHARED_LIB)
     if (UNIX AND APPLE)
         set(CMAKE_MACOSX_RPATH 1)
     endif ()
+    if (NOT TIDY_SO_VERSION)
+        set(TIDY_SO_VERSION ${TIDY_MAJOR_VERSION})
+    endif ()
     add_library ( ${name} SHARED ${CFILES} ${HFILES} ${LIBHFILES} )
     set_target_properties( ${name} PROPERTIES 
                                    OUTPUT_NAME ${LIB_NAME} )
     set_target_properties( ${name} PROPERTIES
                                    VERSION   ${LIBTIDY_VERSION}
-                                   SOVERSION ${TIDY_MAJOR_VERSION} )
+                                   SOVERSION ${TIDY_SO_VERSION} )
     set_target_properties( ${name} PROPERTIES 
                                    COMPILE_FLAGS "-DBUILD_SHARED_LIB" )
     set_target_properties( ${name} PROPERTIES 
diff --git a/build/cmake/build-me.sh b/build/cmake/build-me.sh
index 61bdbdd..a3d7317 100755
--- a/build/cmake/build-me.sh
+++ b/build/cmake/build-me.sh
@@ -35,8 +35,9 @@ TMPOPTS=""
 ##############################################
 ### ***** NOTE THIS INSTALL LOCATION ***** ###
 ### Change to suit your taste, environment ###
-TMPINST="/usr"
+TMPINST="$HOME/projects/html_tidy"
 TMPOPTS="$TMPOPTS -DCMAKE_INSTALL_PREFIX=$TMPINST"
+TMPOPTS="$TMPOPTS -DTIDY_SO_VERSION=6"
 ##############################################
 
 ### Accept user argument

This makes it user settable, but is that what we want... some distributions deciding to, and some more carefully weighing this up, and leaving it as we chose, the default ${TIDY_MAJOR_VERSION}... it is compatible to the previous release...

Used this would result in an install as follows, perhaps a little confusing to some -

lrwxrwxrwx 1 geoff geoff      12 Oct 22 20:17 libtidy.so -> libtidy.so.6
lrwxrwxrwx 1 geoff geoff      17 Oct 22 20:17 libtidy.so.6 -> libtidy.so.5.7.16
-rwxrwxr-x 1 geoff geoff 1078624 Oct 22 20:17 libtidy.so.5.7.16

Unfortunately, I do not feel it is the time to bump the ${TIDY_MAJOR_VERSION} to 6... this seems to promise, suggest, some big changes, which are not present in this release... and thus subsequent up to the current next branch... but could be persuaded...

What do others think? I need more feedback... thanks...

But this does not yet address the privTidy functions... changes in the internal arrangement and use of library only functions must not cause this SOVERSION change... ever... so still need a solution to that... maybe we can default to hiding all functions externally, and use the TIDY_EXPORT to make the public API visible... as done in the Windows import library, so this only applies to UNIX, so need so help to do this... patches, PR, etc appreciated...

@ler762
Copy link
Contributor

ler762 commented Oct 22, 2018

Unfortunately, I do not feel it is the time to bump the ${TIDY_MAJOR_VERSION} to 6 ...

What if #752 / #754 is included :)

If you need to bump the version to 6 maybe that's enough of an excuse?

It'd be nice if the debian people could take a look at that PR and comment. The interesting bit starts here
#752 (comment)

Yes, I oppose having a ${HOME}/.XXXrc file that can't be over-ridden by a command line option.

@cmb69
Copy link
Contributor

cmb69 commented Oct 23, 2018

@ler762 Would any of these affect libtidy?

Anyhow, since tidy 5.6.0 had removed some exported symbols, it's already too late to push to 6.0.0.

@geoffmcl See how the export stuff is done in libgd, which might give some ideas for libtidy.

@geoffmcl
Copy link
Contributor

@ler762, as @cmb69 points out, changes to console tidy.c sample code have nothing to do with libtidy, SOVERSION, and do not appreciate hijacking threads to push/discuss/comment unrelated matters...

@cmb69 thanks for the libgd link... yes I have several examples of sort of how to hide in gcc, but would really appreciate if someone with more feeling for unix offered tested patches, or even a PR... I sort of need to see how big a deal this is...

Like the import library, in Windows, of the .lib/.dll pair, already does, only public API functions should show externally in a shared .so library... who can help with this... thanks...

@oerdnj
Copy link
Author

oerdnj commented Oct 27, 2018

@geoffmcl Ping me in two weeks (5. November) and I might have some time during next IETF, and I'll prepare MR for this.

@ler762
Copy link
Contributor

ler762 commented Oct 28, 2018

@geoffmcl I'm sorry if I came across as trying to hijack the thread. I was responding to your comment about

Unfortunately, I do not feel it is the time to bump the ${TIDY_MAJOR_VERSION} to 6... this seems to promise, suggest, some big changes, which are not present in this release...

with a justification for bumping the version. ... assuming that if you release a new console version of tidy you'd also be releasing a new libtidy version even if nothing in libtidy has changed.

As for

Anyhow, since tidy 5.6.0 had removed some exported symbols, it's already too late to push to 6.0.0.

I'm probably missing something, but it looks like debian libtidy is still on 5.2.0

@tehnick
Copy link

tehnick commented Oct 28, 2018

@ler762 This page is more informative: https://tracker.debian.org/pkg/tidy-html5

@ler762
Copy link
Contributor

ler762 commented Oct 29, 2018

@tehnick thanks for the link. In the News section it has
[2018-10-11] Accepted tidy-html5 1:5.6.0-0.1~exp1 (source amd64) into experimental, experimental (Boyuan Yang)

So it looks like the decision to go with 5.6.0 is less than 3 weeks old. If a new version of tidy is released in the next few weeks can that be changed or is debian set on going with 5.6.0 regardless?

@hosiet
Copy link

hosiet commented Oct 29, 2018

If you are wondering Debian's decision on next stable release: it really depends on to which extent the next tidy-html5 release would break its API.

Well, at least we Debian developers can cherry-pick commits that are important enough from git trunk.

@tehnick
Copy link

tehnick commented Oct 29, 2018

@ler762

@oerdnj and @hosiet are official maintainers of tidy-html5 package in Debian. So almost all decisions will be done by them.

I am just a side observer here. I found this issue only due to current library transition for package tidy-html5 which is now preventing automatic migration of my package psi-plus from Debian unstable to Debian testing (and as a result it also prevents backporting of psi-plus into Debian stable).

[2018-10-11] Accepted tidy-html5 1:5.6.0-0.1~exp1 (source amd64) into experimental, experimental (Boyuan Yang)

This is note about uploading package to Debian experimental branch. Probably you do not know, but this is something like a "play ground" in Debian. In this branch package maintainers may decide to break any parts of packages for testing and debugging purpose.

The most critical part of Debian maintainers work is uploading of packages into Debian unstable branch. Ideally on this step there are no new issues should happen during upgrade of end-user system. (Many people use Debian unstable branch as a "true" rolling release even nowdays.)

So when you are looking at debian/changelog pay more attention to timestamps in blocks with uploads to Debian unstable. Uploads to Debian experimental may be interesting as well, but they are not so important...

@tehnick
Copy link

tehnick commented Oct 30, 2018

Probably you do not know, but this is something like a "play ground" in Debian.

In more details it is described here:
https://wiki.debian.org/DebianReleases#Workflow
https://www.debian.org/releases/
https://wiki.debian.org/DebianExperimental#Introduction

(Just in case if you are curious about it.)

@ler762
Copy link
Contributor

ler762 commented Oct 30, 2018

@tehnick Thanks for all the background info - I appreciate it! And yes

Probably you do not know ... in Debian.

for anything related to administration is most likely true for me.

@hosiet Yes, I am wondering if picking tidy 5.6.0 for unstable now means that version is locked in for the next year or three.

@hosiet
Copy link

hosiet commented Oct 30, 2018

@ler762

That's related to workflow in Debian. Basically it's like this:

  • Library version in Debian will not change in any already-released Debian version (e.g., Debian 8, Debian 9, etc)
  • Debian 10 freeze will happen in January 2019; after that tidy-html5 version in (not-yet-released) Debian 10 will (almost) never be changed
  • If any security problem emerges (e.g., owns a CVE entry), the patch will be cherry-picked and applied to affected libraries in released Debian versions.

So the sooner a new version of libtidy is released, the more likely it would be included in next Debian release. Of course if the API of new libtidy broke too much we may as well decide not to include it for now (since great breakage will break downstream software). Anyway it depends.

@ler762
Copy link
Contributor

ler762 commented Oct 30, 2018

@hosiet thanks for the info! I'm guessing that any more questions I have for this will be pretty much off-topic for tidy. After I read a lot more, follow up questions would best be submitted where?

@hosiet
Copy link

hosiet commented Oct 30, 2018

I'm not sure what kind of questions would emerge, but there are some pointers:

  • About issues of tidy-html5 in Debian: submit a bug report as standard Debian bug reports
  • Private question about tidy-html5 maintenance in Debian: write emails to maintainers publicly listed out

@geoffmcl
Copy link
Contributor

@oerdnj, @hosiet, @ler762, @cmb69, et al...

20210410: Experimentation with trying to hide exporting non-API items from the shared SO library... as they are already in the shared Windows DLL/LIB...

First, in general support bumping the SO number to 6, so this is not about that... and decoupling the SO number from the major version number... all ok...

But to avoid that in future when only libTidy internal functions, and data, are changed, reordered, renamed, removed, added, etc, etc... of course, these internal items will continue to be named using the TY_(name) macro, giving them a clear prvTidyXXXXX name... which is already a BIG clue...

First try, with a simple, but failed -

diff --git a/src/forward.h b/src/forward.h
index 7f59371..5ce01d3 100644
--- a/src/forward.h
+++ b/src/forward.h
@@ -19,7 +19,11 @@
 #include "tidy.h"

 /* Internal symbols are prefixed to avoid clashes with other libraries */
+#ifdef _WIN32
 #define TYDYAPPEND(str1,str2) str1##str2
+#else
+#define TYDYAPPEND(str1,str2) __attribute__((__visibility__("hidden"))) str1##str2
+#endif
 #define TY_(str) TYDYAPPEND(prvTidy,str)

 struct _StreamIn;

BUT it FAILED! Many, many warnings, errors - Ugh

Another try, with - harder, but just many files to modify -

diff --git a/src/forward.h b/src/forward.h
index 7f59371..4ce8ca5 100644
--- a/src/forward.h
+++ b/src/forward.h
@@ -22,6 +22,13 @@
#define TYDYAPPEND(str1,str2) str1##str2
#define TY_(str) TYDYAPPEND(prvTidy,str)

+/* Internal symbols are prefixed with 'hidden' attr, to avoid exporting */
+#ifdef _WIN32
+#define TY_PRIVATE
+#else
+#define TY_PRIVATE __attribute__((__visibility__("hidden")))
+#endif
+
struct _StreamIn;
typedef struct _StreamIn StreamIn;

Then adding TY_PRIVATE to the header files containing <return> TY_(function)(<params>); prepend these with TY_PRIVATE... Also did this in some *.c files, or added static in front of others, if only used in that one module...

This reduced the original over 1000 prvTidy<function> in the readelf -Ws libtidy.so.5.7.45 list to some 360, excluding the TY_(W3CAttrsFor_XXXX)[] data list, about 270, which I found no easy solution to... so this seems a significant improvement...

This was at a cost of modifying some 24 files, adding some 370+ TY_PRIVATE to the function declarations...

20210412: pushed issue-743 branch, with this major change...

Using $ nm -D <lib.so>... ie dynamic symbols...

The current next (5.7.45) shared SO libtidy.so.5.7.45 outputs some 555 ' T ' exported functions... some 361 have the name `prvTidyXXXX', which should not be exported... are NOT part of the PUBLIC API!

The current issue-743 branch shared lib only outputs some 194 ' T ' exported functions... none have the name `prvTidyXXXX' == some success ;=))

There are however 12 that do not commence with the usual tidyXXXX API signature...

    • 2 begin TidyXXXX - TidyLangPosixName and TidyLangWindowsName
    • 8 begin getXXXXX - getErrorCodeList getInstalledLanguageList getNextErrorCode getNextInstalledLanguage getNextStringKey getNextWindowsLanguage getStringKeyList getWindowsLanguageList
    • 2 should not be there - formatDialogue and inRemovedInfo - both can be removed by adding static to the function declatation... done... no probs...

The remaining 10 (2 + 8), are just anomolies in the libTidy PUBLIC API - sad, but (I think) too late now to change... just accept them... and try not to repeat this...

There are some 138 of the form prvTidyXXXXXX...

One(1) is marked with a ' D ', by nm - prvTidyg_default_allocator, which should be hidden, only exported to other libTidy modules - done, now fixed... using TY_PRIVATE...

The remaining 137 of the form prvTidyW3CAttrsFor_XXXX[], which are arrays of attributes, supported by the XXXX element, shown with ' R ' by nm... How to hide these?

So far, my experiments using TY_PRIVATE on these 137 have all FAILED! UGH! NEED HELP

But, this issue-743 branch represent a major step forward - I think - and paves the way for a version 5.8.0, SO=6, release...

Look forward to further feedback, comments, patches, etc, etc... to solve the outstanding 137 items... thanks...

@geoffmcl
Copy link
Contributor

Seem to have solved, and pushed, the fix for the last 137...

Trick was to keep the extern attribute... seems no warnings, errors... and no dynamic export...

Appreciated if others could checkout this issue-743 branch, test, and report... will try to get around to setting up a PR, once tested... thanks...

@balthisar
Copy link
Member

@oerdnj, it looks like I can take this an run with it. I'm sorry that this issue has been hanging around for so many years; I've not had a lot of opportunity to be able to step in an help @geoffmcl, who is very much a Windows person.

I get along on Linux just fine, but package management is a black art to me, so bear with me.

It looks like @geoffmcl has gone to some pains to hide some internal private symbols that escaped into the ABI somehow. If this goes live, then I understand we need to bump the SONAME; is that correct?

Aside from being an integer and increasing, are there any other restrictions to the SONAME? HTML Tidy is using a modified version of semantic versioning, so if the SONAME is flexible enough, I think it would be quite easy to manage.

Because 5.6.0 is the current, stable, officially-released version, it's the only one that we guarantee API/ABI stability for. If we patch it to 5.6.1, we guarantee full compatibility. This, can we publish this as libtidy.so.56?

We guarantee this stability for every minor release. There will never be a 5.7.x stable release; when we're ready, we will release our master, stable as 5.8.0, and again guarantee the API in all 5.8.x releases. Our next branch then becomes 5.9.x.

This means that our next branch version 5.7.53, which is the current version, offers no guarantees of stability between minor versions. It's our development branch, and is always in flux, and probably shouldn't be in anyone's package management system because by our very own definition, this isn't stable. Thus, our build process might set the SONAME to 57, but it won't mean anything, because package maintainers should be looking at master and not next.

Does this seem like a workable approach?

If so, I'll merge @geoffmcl's changes to hide the symbols, and prepare a new, stable 5.8 release with a SONAME of 58.

Also pinging @hosiet, @ler762, @cmb69, who were also participating in this thread.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants