On 28 April 2016 at 15:28, Yury Gribov <[email protected]> wrote: > On 04/28/2016 04:59 PM, Emil Velikov wrote: >> >> On 27 April 2016 at 08:48, Yury Gribov <[email protected]> wrote: >>> >>> On 04/26/2016 02:14 PM, Emil Velikov wrote: >>>> >>>> >>>> On 26 April 2016 at 07:54, Yury Gribov <[email protected]> wrote: >>>>> >>>>> >>>>> On 04/20/2016 10:03 AM, Yury Gribov wrote: >>>>>> >>>>>> >>>>>> >>>>>> On 04/19/2016 06:35 PM, Yury Gribov wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 04/18/2016 06:21 PM, Adam Jackson wrote: >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, 2016-04-18 at 09:23 +0300, Yury Gribov wrote: >>>>>>>> >>>>>>>>> Does below look like a sane approach? >>>>>>>>> 1) get all deps via >>>>>>>>> $ apt-cache rdepends libice6 libice-dev >>>>>>>>> 2) unpack each of the above .debs and scan for ELFs >>>>>>>>> 3) for each ELF see if one of now hidden symbols is UND >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> That sounds good to me. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> I've cooked a simple script (attached, reviews welcome) and applied >>>>>>> it >>>>>>> to Ubuntu 14. It showed that there are additional uses for >>>>>>> _IceTransNoListen (mentioned by Alan in separate email) but nothing >>>>>>> else. >>>>>>> >>>>>>>>> Note that this does not handle transitive dependencies e.g. if some >>>>>>>>> weird library links static version of libICE and the re-exports >>>>>>>>> it's >>>>>>>>> symbols as part of new lib's public interface. >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> It'd be possible to scan for this too I suspect. Look for packages >>>>>>>> that BuildRequire libice6-static, scan the resulting binaries for >>>>>>>> exports. I suspect there are zero such packages. >>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> I have postponed this activity for now (especially given that this >>>>>>> behavior would be a serious packaging abuse). >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> Here's an updated patch with added _IceTransNoListen. Does this look >>>>>> better now? >>>>> >>>>> >>>>> >>>>> >>>>> Ping. >>>>> >>>> Pong. Sending patches as attachments is not the most productive way to >>>> get things reviewed ;-) >>> >>> >>> >>> I see. Strangely the Content-Disposition in my previous email is properly >>> set to "inline". I've both inlined and attached the new patch to be sure. >>> >> Close but no cigar :-P Please follow the suggestions in the wiki >> http://www.x.org/wiki/Development/Documentation/SubmittingPatches/ > > > I've checked them (from the very beginning actually) and there seems to be > no strong requirement for attachments (it even says "Always attach patches > as plain text files - if emailed then either attached or in-line."). > Looks like you read more than me - I never got past the first 15 lines (the ones mentioning git send-mail). Sorry about that had no idea that the docs are so ... interesting
>>> +have_visibility=disabled >>> +if test x$SYMBOL_VISIBILITY != xno; then >>> + AC_MSG_CHECKING(for symbol visibility support) >>> + if test x$GCC = xyes; then >>> + VISIBILITY_CFLAGS="-fvisibility=hidden" >>> + else >>> + if test x$SUNCC = xyes; then >>> + VISIBILITY_CFLAGS="-xldscope=hidden" >>> + else >>> + have_visibility=no >> >> If Cygwin/Mingw does support the visibility flags (see below) I'd just >> warn/error out in here. >> IMHO if a specific platform/compiler is not handles we better get a >> report and sort it out. > > > But what would we do if there are problems with some > compiler/version/platform (except for configure-time testing)? > Sort it out accordingly as we know a bit more about the users. Which reminds me -> one should update the .pc file as well -> add xproto to the Requires.private section. >> In general I would not bother with anything more than >> - PKG_CHECK_MODULE(XPROTO, xproto) + wire up the XPROTO_CFLAGS (both >> missing) >> - Add the above "if test $compiler set VISIBILILTY_FLAGS" + propagate >> the latter throughout (both done) >> - Add (now missing) #include <X11/Xfuncproto.h> in the respective >> places and annotate the functions (already done) >> >> No extra configure toggles (above), no configure time testing. Unless >> cygwin/mingw requires the latter. >> So the latter is needed. Fair enough. >>> + fi >>> + fi >>> + if test x$have_visibility != xno; then >>> + save_CFLAGS="$CFLAGS" >>> + proto_inc=`$PKG_CONFIG --cflags xproto` >>> + CFLAGS="$CFLAGS $VISIBILITY_CFLAGS $proto_inc" >>> + AC_COMPILE_IFELSE([AC_LANG_PROGRAM([ >>> + [#include <X11/Xfuncproto.h> >>> + #if defined(__CYGWIN__) || >>> defined(__MINGW32__) >>> + #error No visibility support >> >> >> Are you sure that think this is correct ? Afaict things should work, >> despite that xserver sets SYMBOL_VISIBILITY=no. >> >> Jon, any ideas why xserver disables -fvisibility=hidden ? Should we >> remove that line or there's something subtle that requires the >> behaviour ? For example: relying on symbols that are not marked as >> exported or alike. > > > Note that _X_EXPORT is simply not defined for Cygwin and Mingw so there are > basically no annotations at all. They ignore visibility annotations and warn > on them by default: > visi.c:0: warning: visibility attribute not supported in this > configuration; ignored > Most OSS projects disable visibility on Cygwin/Mingw for this reason. > Yes, as mentioned earlier, one should not be using attribute visibility but __declspec dllexport/dllimport for Windows. As to why they're missing is beyond me. So all in all, even though some of my suggestions did not work out there's still a few bits missing in the patch. Thanks Emil _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
