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

[Daniel Stone]

On Tue, Feb 01, 2005 at 12:10:19PM -0500, Mike A. Harris wrote:
> On Wed, 2 Feb 2005, Daniel Stone wrote:
> >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.
> 
> Using this logic, every single symbol that isn't explicitly 
> hidden, is public, and should now be moved to be a public symbol.  
> That would pretty much expose most if not all internal symbols of 
> all of the X libraries, as they're not hidden with 
> attribute((hidden)).
> 
> That is a bogus argument.  All of the internal symbols in various 
> libraries were _intended_ to be internal, and wether they are 
> explicitly hidden or not, they should be treated as private, 
> wether or not that is enforced by the compiler currently.
> 
> Otherwise, with your logic, where does one draw the line?

I invite you to:
	* revert my commits,
	* change _IceGetPeerName to attribute((hidden)),
	* watch the build break,
	* fix the build breakage, either by unhiding this symbol, and
	  making it a part of the public API, or removing libSM's
	  dependency on it.

By the way, I meant 'using published headers'.  Cheating with your own
prototypes doesn't count.

> >Again, I challenge you: if it's private API, remove libSM's dependency
> >on it.  Consider it a documentary clarification of existing fact.
> 
> You're missing my entire point.  You are looking at this entirely 
> as a specific issue of this one specific change.  I do not know 
> wether or not the libSM dependancy can be removed or not, and I 
> don't really care to be honest.
> 
> My concern, is that random developers are making changes to the 
> intended visibility of internal X library symbols on a personal 
> whim without any pre-discussion on xorg mailing lists, and 
> without any peer review.  I consider this practice to be 
> unacceptable by anyone, without discussing the changes on the 
> xorg mailing lists FIRST.  If the change truly is safe and sane, 
> and makes total sense, then there isn't likely to be much 
> objection is there?  Please ignore the specific change you just 
> made when answering this question, and look at the bigger picture 
> of anyone randomly changing any other symbol in any other 
> library.

While I'm honoured by 'random' and 'whim', I assure you that there was
a reasonable amount of thought put in to it, and that it was done with
knowledge of, and in accordance with, proper shared library versioning
strategies.

I like the idea of bureaucracy for managing, say, the branch.  I dislike
the idea of this for HEAD.

You might be surprised to know that I did this a while back with a heap
of patches.  A few of them got merged into HEAD after ajax got sick of
having had them hang around for weeks in Bugzilla and merged them; the
rest are waiting for me to get sick of it also and find the time to
integrate them all.

The problem with doing this is that if you treat no action as implicit
consensus, aside from the fact nothing ever gets done, when do you
accept that you have implicit consensus and move ahead?  A week?  Two?
What if someone pops up after that because they've been on holidays and
cracks the sads because they weren't consulted?

It's a nice idea, right, but in practice, let me assure you that it
falls apart.  It would've just been more noise on xorg@, and I get the
sense that I've been responsible for enough of that already.

Would you have opposed this commit if it was raised?

> Unless there is a standard process that these type of changes 
> have to go through, then people will end up eventually making 
> changes that break compatibility, or cause other damage that 
> can't easily be fixed in the future.

If people make changes that break API or ABI stability, I'm going to
go beat them with the soversion stick until they decide whether to
bump the soversion, or revert the changes and do them in such a
way that API and ABI stability is retained.  I bumped the minor
revision in the soversion of libICE because full backwards compatiility
was retained, but an extra symbol was added to the public visibility
list.

I will be keeping an eye on the commit list as much as I can to see if
I can't try to keep this sort of practice enforced; I have in theory
been doing this on the modular tree, except all that's happened there
has been syncing from the monolithic tree (ironically, the monolithic
commit I did was synced from the modular tree).

> If other X.Org developers feel the same way as I do about this
> concept, then I would like to hear them stand up and say so.
> 
> If other developers think I am wrong, and that it is ok for
> anyone with CVS commit access to change the visibility of library
> symbols without discussing them on the mailing lists first, then
> I would like them to stand up and say so.

I think you're presenting a false dichotomy of either we all go through
a process involving people with beards and pipes, conference calls, and
the word 'ISO'; and a land where everyone gets jacked up on speed,
damages the API in the way that seems least sensible at the time, all
the while going TEEHEEHEEILOVERANDOMAPICHANGES.

> If other developers are completely indifferent and think everyone 
> can judge for themselves when making a change, then I'd like them 
> to stand up and say so also.

I would certainly hope that anyone entrusted with commit privileges to
xorg can make decisions like this.

> >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.
> 
> I wasn't suggesting that you stay out of xorg.  What I'm
> suggesting, is that you discuss these type of changes with other
> developers in the open public forums ahead of time and see if 
> people have a general concensus that the changes are sensible, or 
> if they have specific concerns about them.  This is not about the 
> specific change, but about the idea of someone changing something 
> of this nature without discussing it previously with others.

I honestly don't see the point.

> >It's not introducing new symbols, it's not breaking backwards
> >compatibility, it's not doing anything radical.
> 
> You're focussing on the specific change, rather than what I'm 
> talking about.

So you're saying that, had I asked for consensus, this message would
have foundered on xorg@ until I had forgotten about it?

> >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.
> 
> If you choose to see my concerns as fighting you, then you're 
> being overly defensive.  Any project of this size, with as many 
> developers as X.Org has contributing to it, has to have some 
> sanity to it all.
> 
> Unfortunately, there are no real written CVS policies that detail
> acceptable versus unacceptable changes, or detail what should be
> discussed amongst developers first and concensus reached before
> changes occur.  Without having any guidelines or controls in 
> place, every developer is completely free to make whatever random 
> changes they see fit, and hope nobody notices.

I would hope that 'don't be a dick' would suffice as a written policy.

As far as those go, we have unwritten, but adhered-to, policies on
changelogging commits; we presumably follow sanity with our shared
libraries in terms of soversions; I assume people wouldn't commit things
with random dodgy licences[0], or which they didn't have copyright to;
you wouldn't make a change which maliciously broke stuff, et al.

> I totally support project openness, modernization, am quite
> accepting of changes, and am also all for easy access to CVS
> write priveledge to the masses including you, in order for it to
> "work" and "work well" for the project as a whole, and all of the
> participants involved, is by having good communication between
> each other, good working relationships, and erring on the side of
> caution by discussing certain types of changes with others prior
> to making them.  If you can forget about this particular change
> for a minute, would you agree that these are in general, good
> ideas for people to follow?

Sort of.  I forsee an xorg@ which is useless because everyone's running
around trying to get permission for some tiny fix to something else,
and eventually everyone just abandons the swamp that is xorg@, so people
just start blindly committing whatever they see fit.

I really can't see the need for approval here -- not in this specific
case, not in general.  Shared libraries are fragile, and notoriously so,
so I would hope that someone who didn't know their way around shared
libraries would either ask on the list, or ask someone they knew to
check their patch for them, but in the general case, I don't think it's
necessary to continually babysit developers[1].  We're working in HEAD,
no-one will ship a pre-compiled Matlab or something built against HEAD.

If anyone came up with any specific concerns, it could've quickly been
reverted with no damage done.

[0]: The current font mess being a historical exception.
[1]: If we have developers who need babysitting, maybe their
     developership needs to be reviewed.
-------------- 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/5c03faaa/attachment.pgp>