Changing private symbols to public (was: Re: CVS Update: xc (branch: trunk))

[Daniel Stone]

On Tue, Feb 01, 2005 at 07:00:01AM -0500, Mike A. Harris wrote:
> On Tue, 1 Feb 2005, Daniel Stone wrote:
> >On Tue, Feb 01, 2005 at 12:35:03AM -0500, Mike A. Harris wrote:
> >> I searched the mailing list, and I couldn't find any emails 
> >> mentioning the rationale or approval for this change.  Was this 
> >> discussed on a confcall or somesuch?
> >
> >No, it wasn't.
> 
> IMHO, such API changes should be discussed and concensus reached 
> before any changes occur.

It's not any change from the status quo.  Honestly, if it's private
API, then by definition, libSM was never physically able to link to it.
By the fact it was, means that it was public API.  This was adding a
symbol and bumping the soversion to account for this fact.

> >> Could someone summarize the rationale for the change, and it's 
> >> acceptance into CVS head?
> >
> >The rationale is simple: if another library is using it, it is not
> >private API, no matter how many underscores you put in front of the
> >symbol, and no matter that you put a comment saying 'this is private
> >API'.  It's simply not.
> 
> I don't disagree with that.  X.Org needs to be enhanced to hide 
> it's internal library symbols using gcc's attribute((hidden)) 
> functionality (and potentially other compiler's similar 
> functionality).  Jakub Jelinek wrote some stuff for XFree86 
> before to do this which nobody ever bothered integrating for 
> whatever reason.

Christ.  I'd love to dig that up and integrate it.

> >I made this change so we could actually delineate between what
> >was and was not private API.  I don't know what the rationale
> >for intruding on another library's allegedly private API was in
> >the first place, but I just made it official: either it should
> >be public, or libSM must not rely on it.
> 
> This indicates that there is indeed a problem, and that someone 
> should investigate potential solutions for the problem, discuss 
> the problem openly on the mailing lists, and get other people's 
> thoughts on weighing the ups and downs of the different potential 
> solutions.
> 
> Then, once concensus can be reached between the parties 
> discussing the potential solutions, whichever solution is 
> decided, can be approved and changed officially.
> 
> I don't think it is a good idea for random developers (whoever 
> they are) to just go ahead and make API changes of this nature 
> without consulting other developers who will be potentially 
> impacted by the changes.

Again, I challenge you: if it's private API, remove libSM's dependency
on it.  Consider it a documentary clarification of existing fact.

> >I committed it into CVS HEAD as it was quite obviously correct.  I
> >moved the symbol into public API visibility, and bumped the soversion
> >by 0.1 as it introduced a new symbol.  Backwards compatibility was
> >retained.
> 
> You consider it correct obviously, or you wouldn't have committed 
> the change.  So far, nobody has stated the change is incorrect, 
> nor pointed out any problems with it, and it may very well turn 
> out to be deemed the correct change by everyone, and left in 
> tact.
> 
> The point I'm trying to make, is that no discussion about this 
> change ever took place on any mailing list or other forum that 
> I'm aware of.
> 
> Does this establish precedence that it is ok for anyone with CVS 
> commit access to change the X library API's in any way they deem 
> to be "obviously correct" without any pre-discussion between 
> other developers?
> 
> I think we need to establish some CVS policies to ensure that any 
> changes of this nature are adequately discussed and agreed upon 
> by multiple developers via peer review of some sort.
> 
> Again, my point isn't necessarily about the specific change, but 
> rather the fact that it occured without discussion.
> 
> Comments/feedback from anyone and everyone appreciated.

I'm happy to stay out of the xorg pissing pond, as it were, and stick
to xlibs if that would make you happy, but I honestly don't think that
changes of this nature impact anything at all.  If you want to remove
it from the public API: well, for one, you're too late; for another,
remove libSM's dependency on it.  As I've stated before, this was
merely a documentary clarification of existing fact.

It's not introducing new symbols, it's not breaking backwards
compatibility, it's not doing anything radical.  It's correcting an
unfortunate misstatement somewhere along the line.  When libSM started
using _IceGetPeerName, the person who did that made a conscious decision
to move that symbol into the public API.  It has not been private API
since that point, and the existing situation served only to lie about
its true visibility.  Honestly, if you want this fight, find whoever
started polluting libSM with ICE internal API and take it up with them,
but I don't care enough, and I'm beginning to wish that I had never
bothered attempting to enforce sanity in the monolithic tree, and just
staying with the modular tree instead.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.x.org/archives/xorg/attachments/20050202/c0ad1321/attachment.pgp>