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

[Mike A. Harris]

On Wed, 2 Feb 2005, Daniel Stone wrote:

>> 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.

Daniel, I'd like to make sure that you know that I am not opposed 
to the change you made, as you seem quite on the defensive about 
that, and I wasn't trying to be on the offensive.  I did not have 
any opinion as to wether or not the change itself was right or 
not, I just wanted to see the discussion that led up to the 
change so I could understand the rationale myself, etc.

In hindsight, I've went over the thread from the start again, 
and I believe that I could have reworded myself in a better way 
so that my intentions were more clear.

I did not mean for this to come off in a personal way, nor for
you to be offended by it, and I apologize if that has happened.


>> 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.

The discussion since my original posting, including comments from
keithp and yourself have convinced me now that there was a fair 
amount of thought put into this in the modularized tree already, 
including rationale.

It was the rationale behind the change which I wanted to know, 
and I feel that has now been presented finally, and that the 
rationale given seems rather sensible.

However, I do want to point out that until this discussion took 
place, the change that occured did come all of a sudden, and with 
no prior discussion that I could see, and so it really did seem 
to be a random whim.  Now that it has been clarified that this 
was not the case, I beleive the miscommunication has been 
corrected, and we are closer to being on the same page.


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

I dislike bureaucracy completely.  I do however like to see open
discussion on all aspects of development, and I encourage people
working on (any) code to discuss their ideas/plans, etc. ahead of
time with others, as that quite often yields new ideas, etc. and 
also gives the principle of least surprise.

I do not think that there should be a hardcore strict requirement 
that everything be discussed ad infinitum, and voted on by some 
bureaucratic panel for 6 months.  That would be counterintuitive 
to the goals of the project, and would be going backwards rather 
than forwards.

The project needs to have more freedom of that, and with freedom 
comes responsiblity of everyone to use best judgement with all 
decisions they make.  Sometimes there will be universal 
agreement, and sometimes not.  Sometimes there will be reasonable 
concensus on things and sometimes not.  Sometimes things can be 
put into CVS without anyone questioning them, and other times 
people will question things that go into CVS, in particular if 
they have a concern about it and can't find any previous 
discussion anywhere about the item in question.

I definitely hope that all people contributing to the code and
checking things into CVS review other people's commit messages,
file modifications, and review code diffs for errors, crackrock,
and other things.  Since we do not have a dictator controlling
what goes into the tree, we _all_ need to watch what goes into
the tree, and I believe voluntary code review by everyone who has
the time to do so, will help detect potentially bad things from
getting into the tree.

When someone sees a change that they disagree with, or have
concerns about, or just don't understand, they should speak up 
and bring their concerns, so that it can be discussed and 
resolved, and preferably in a friendly polite manner.

Having said that, I believe I could have approached my concerns 
from a different angle which perhaps would not have been taken in 
as bad a way.


>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.

I'm not surprised per se, but it is nice to be made aware that 
other work has been done, or will be done in a similar vein as 
well.


>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?

Yes, this is a problem, and I have faced it myself many times, 
including with X.org CVS commits, and with a previous popular 
X11 implementation as well.  I personally despise the "assume 
your changes are ok if nobody objects" line of thinking because 
it makes the assumption that:

	no response == acceptance and implicit permission

When in reality, people who might object or have a problem with 
the changes might be on vacation, or have missed the email, or 
any number of things.  Maybe they just didn't have the time to 
look at it and even make a judgement.

That is irritating indeed, and I've had various things sit in
different bugzillas for long periods of time, and never get any
feedback good or bad.  I too have frequently gotten a feeling of
"I'll just commit it then and if anyone has a problem they can
bring it up then".  And I've went ahead and made some CVS commits 
having not received any objections, only to have people object 
AFTER the fact a day or 2 or 5 later, which was very frustrating 
because I had moved on to other stuff, and had to come back to 
the issues again.


>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?

Now that I know the rationale behind the change, and that it had 
a fair amount of thought put into it, and that thought has been 
expressed in reasonable detail, I would have to say that had this 
information been discussed on the list ahead of time, and I had 
an opportunity to review it, I would not have any objections.


>> 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.

Then we are in violent agreement on that.


>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).

This is also good to hear.  The more people who watch commit
messages and/or review diffs of things commited, and voice
any concerns, the better.


>> 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.

No, that is not what I want to see, but I can see how my 
statements might have mislead you to think that's what I meant.  
I assure you I dislike confcalls with beards and pipes and the 
word ISO.  I prefer meetings where people "talk about shit" over 
meetings where people "bring a motion to the table", and I 
believe you'll probably "<expletive deleted> right!" me on that 
before you'd "concur" on that.


>> 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.

We can all hope that, but we've also seen changes occur that have 
had objection by one or more parties in the past, some of which 
had to later be modified in some way to become accepted, or which 
were reverted or disabled.  I don't think this is reason to 
remove any of those people's CVS commit priveledges.

I think that with freedom comes responsibility, but also that
everyone must accept the fact that from time to time someone else
will object to their changes, and possibly want the changes
reverted or explained, etc.   I do not propose to revoke anyone's 
CVS priveledge due to a disagreement arising between people over 
a commit, unless the commit is highly controversial or breaks 
some major taboo, etc.


>> 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.

Perhaps because you had already discussed it with keith and 
others in the modular tree?  I guess it's kindof moot either way 
now though, as I think we're more in agreement than anything.


>> 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.

It would be nice to hope for that, but in practice I don't think 
implicit policies are good enough.  I don't think a 20 page 
manual of "do this/dont do that" written as an ISO document is 
acceptable either.  Something like the DRI project's CVS policy 
page is what I have in mind, and it is pretty simple and basic 
common sense IMHO.


>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.

Unwritten policies are a bad precedent though, as there will 
always be new people coming along who don't know what the 
unwritten policy is.  I face a lot of that every day.  I strongly 
believe that balance is needed between explicitly stating things, 
and letting things be implicit.  In particular, it is a good idea 
to formalize some explicit things, when there have been problems 
in a given area at some point, or when people frequently seek 
clarification about something in particular.

A CVS policy document should be mostly a guideline to avoid 
problems, and inform people of some minimum sensibilities to 
follow.  Focus being on "guideline".


>> 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.

That would not be productive.


>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.

I wasn't suggesting that your change needed approval.  Approval 
implies that some official body (the arch board?  the BOD?) would 
have to vote or some crackrock like that.  "Concensus" is much 
more informal discussion, which might even end up with nobody 
providing any feedback at all.  If you decided to post to the 
list asking for concensus, and not gotten any feedback at all, 
and then decided to make the change, then you could yell at 
people for complaining later.  ;o)  Also, there would have been 
something to find with the list search engine. ;)

However, I agree with the prinicple of HEAD being more volatile 
by nature of what it is.


>[1]: If we have developers who need babysitting, maybe their
>     developership needs to be reviewed.

Overall, I don't think anyone needs babysitting, but that doesn't
mean that nobody will ever object to things that get committed to
CVS either.  And that's perfectly ok, as long as any objections
or concerns get resolved via discussion or whatever afterwards,
like this one has now.  ;o)

Take care.



-- 
Mike A. Harris, Systems Engineer - X11 Development team, Red Hat Canada, Ltd.

IT executives rate Red Hat #1 for value:  http://www.redhat.com/promo/vendor