Accepting contributions and merging code is pretty basic. Review of code is going to take place at some point.
I pay much more attention to development in Second Life viewers and OpenSIM code. Over the last two years I've seen two major exploits. One was solely in the open source side of viewer (client) development. A group of developers created a version of OpenJPEG that performed better. Once it was included as a reliable binary for the main program to link to, the library was changed again. The source was 'sort of' hidden and several functions were obfuscated. The code changes allowed some of the team to have the program embed user information in images baked in the viewer and sent to the servers (avatar appearance and user made textures). This allowed specialized viewers privately compiled and made to look like the standard viewer to retrieve the embedded information. Only when someone began wondering how a small group of people seemed to have knowledge about players that should not be available did the scam come to light.
The point is the casual code reviews in place at the time did not protect the user community. Members of the developer's team were able to hide an exploit from other team members. The result is that team was destroyed. Plus a number of people were permanently banned from the community. Faith in open source code was shaken, rightfully or wrongfully.
Code is now developed in the community with much stricter review rules. OpenUru.org does not need to repeat that mistake.
Since the information being stolen from users computers could be used to hack their computer and crack their game accounts (and the recent scam - to crack a users password style and then use it for ID theft) there was considerable dollar value at stake. Nor were the people solely targeting the game or game accounts. Users RL back accounts were the long range target.
People are allowing a program into their computer and past their virus protection. Many in the community are not the techies that can understand how to isolate a program and monitor what it is send out of their computer. Data embedded and hidden within other data is difficult to find and ups the techie factor needed for discovery. Somewhere there has to be some level of supervision and review.
Allowing any unreviewed code to merge into the main branch seems like an enormous risk. Private branches can do whatever they want.
Getting the Ball Rolling
Re: Getting the Ball Rolling
Nalates
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
Re: Getting the Ball Rolling
Code Reviews help identify ways to make the code better.
Code Reviews can uncover problems too but that is not their primary purpose; see above.
Code reviews are a form of quality audit.
There are environments where quality audits are done for the wrong reasons and with the wrong attitudes. I feel empathy for people who think that audits are meant as a form of babysitting or punishment, because that tells me they're learning bad lessons from even worse management.
I expect we can agree that few people want to work in an environment of distrust, and that micromanaging and babysitting are poor ways to manage people. I hope that we can also agree that none of us are perfect, that unless one of us is Donald Knuth that none of us write perfect code.
Code reviews offer an excellent way to reinforce best practices and approaches, and expand our knowledge by learning from each other. If anyone among us thinks you already know everything and there's nothing that you could possibly learn from others, please raise your hand so we can begin the Nobel Prize nominations promptly.
Code Reviews can uncover problems too but that is not their primary purpose; see above.
Code reviews are a form of quality audit.
There are environments where quality audits are done for the wrong reasons and with the wrong attitudes. I feel empathy for people who think that audits are meant as a form of babysitting or punishment, because that tells me they're learning bad lessons from even worse management.
I expect we can agree that few people want to work in an environment of distrust, and that micromanaging and babysitting are poor ways to manage people. I hope that we can also agree that none of us are perfect, that unless one of us is Donald Knuth that none of us write perfect code.
Code reviews offer an excellent way to reinforce best practices and approaches, and expand our knowledge by learning from each other. If anyone among us thinks you already know everything and there's nothing that you could possibly learn from others, please raise your hand so we can begin the Nobel Prize nominations promptly.
The music is reversible, but time is not.
Re: Getting the Ball Rolling
(This post is separate from the Knuth discussion that erupted, so I can return to the original topic at hand)
Last night I couldn't finish my thoughts before drifting into blissful sleep, so here's a second helping!
Nalates highlighted an issue among the Second Life Viewer dev community, and made a compelling argument for code review as a solution to the problem of untrustworthy developers. However, from my perspective, exploitation is not the problem; it is a symptom of an environment that allows (perhaps encourages) such behavior to develop in the first place.
I return to the premise that it's all about the management style. If the management is poor (micromanaging, creating an environment of distrust and walled gardens), then code reviews will be seen negatively. They become a weapon rather than a tool. But if management focuses on the benefits (an environment of increased trust, respect, growth, and professionalism), the outcome will be different.
Reviews among peers and SMEs (subject matter experts) help to foster collaboration skills (always a plus) and allow participants to take on the roles of student and teacher, often interchangeably. Imagine the world in which software developers say to one another, "I'd be happy for you to review my work. Something that may be obvious to me may not be obvious to others, so your input could make my work more maintainable. I might learn something. And if you have questions, maybe you'll learn something too. Together, we can find more optimal solutions."
When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group.
That's a different way from looking at it as "You want to review my code because you don't trust me!" isn't it?
Last night I couldn't finish my thoughts before drifting into blissful sleep, so here's a second helping!
Nalates highlighted an issue among the Second Life Viewer dev community, and made a compelling argument for code review as a solution to the problem of untrustworthy developers. However, from my perspective, exploitation is not the problem; it is a symptom of an environment that allows (perhaps encourages) such behavior to develop in the first place.
I return to the premise that it's all about the management style. If the management is poor (micromanaging, creating an environment of distrust and walled gardens), then code reviews will be seen negatively. They become a weapon rather than a tool. But if management focuses on the benefits (an environment of increased trust, respect, growth, and professionalism), the outcome will be different.
Reviews among peers and SMEs (subject matter experts) help to foster collaboration skills (always a plus) and allow participants to take on the roles of student and teacher, often interchangeably. Imagine the world in which software developers say to one another, "I'd be happy for you to review my work. Something that may be obvious to me may not be obvious to others, so your input could make my work more maintainable. I might learn something. And if you have questions, maybe you'll learn something too. Together, we can find more optimal solutions."
When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group.
That's a different way from looking at it as "You want to review my code because you don't trust me!" isn't it?
The music is reversible, but time is not.
Re: Getting the Ball Rolling
In Karl Fogel's Producing Open Source Software (JW had a link which I don't have time to search for right now), in Chapter two, there are some admonitions about using reviews to foster openness. In the experience he described, the value of reviews only became widely accepted after one of the team members took on the task of reading every change that went into the project, and always commenting constructively on the changes. Lead by example.
Most of the posts in this thread support a review element to our process. Maybe it's time to identify some of the critical elements (benefits) we expect to reap from reviews. Here's a short list:
Most of the posts in this thread support a review element to our process. Maybe it's time to identify some of the critical elements (benefits) we expect to reap from reviews. Here's a short list:
- Change meets a stated goal, does not exceed goal's scope
- Change is technically sound
- Change includes documentation on reason for change, functional summary, technical summary (API)
One of the OpenUru toolsmiths... a bookbinder.
Re: Getting the Ball Rolling
Here's the link to Karl Fogel's free Producing Open Source Software. It is recommended reading.
http://producingoss.com/en/producingoss.pdf
http://producingoss.com/en/producingoss.pdf
Perfect speed is being there.
Re: Getting the Ball Rolling
Marten’s view of environment has a great deal of merit. Anyone that has read Carl Rogers’ studies and research in psychology understands that our perceptions of and attitudes about a person shifts their behavior. Studies revealed a feedback loop directly related to attitudes held with a certainly significance in the high 90’s. Few people outside the field of psychology know that the Egyptian-Israeli agreement came in part from Pres. Carter having counselors from Roger’s group assist the negotiations between people with deep racial hatreds. Attitudes have far more effect than most imagine or many can believe.
Getting the mind set for OU into a positive place and keeping it there falls to the leaders in the group. Here JW and Mac set the foundation attitudes. Those supporting them provide some personal spin to the attitudes, but that is like planets orbiting a sun. They never get too far away from the core without leaving the group. So, I think OU has a good foundation.
Where I depart from Martin is at his statement, "When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group."
Respect, trust, transparency… they create a good environment. They do not exclude 'good intentions' gone bad. Going a bit deeper into the Emerald scandal for an example, several of the developers on the Emerald Team were creating content for SL. That content sold for RL money and was being stolen. They saw a problem.
Members of the team began to look for a way to stop the thieves taking their money/stuff. That grew into the Onyx Project and then that grew into other protection and tracking tools. They filled a need. Then people from outside the group figured out what was going on and took advantage of the code within the Emerald viewer. The result was Emerald users were at risk because of code that made it into the viewer for reasons thought to be good.
I’m not discounting Marten’s points on the importance of attitude, respect, trust, and transparency. But, they are not an absolute or even a good defense. They lead to the Emerald Teams's downfall. The people that actually placed the code into viewer did so for what some consider noble reasons.
In our community we have idealistic arguments over Drizzle. People see it differently. People presented with the same scenario come to different positions. For this… reason… attitude, respect, and trust are not impregnable defenses nor particularly good ones. Great for motivation and creativity and teamwork, but not so much for defense.
Putting all the esoteric reasoning aside, things can be simple. Review the code because it is vulnerable to abuse and mistakes. Not doing so would be disregarding the safety of those using the product. Keep everything transparent and be aware that at any moment someone may have a misguided idea and act on it and more likely just make a mistake.
There are people that constantly look for ways to get passwords. As small as Uru is, it is still a source of access to people’s computers. We will have people coming in who may have hidden agendas. Still we want to welcome and trust new people. But, verify their code.
Rarified’s goals for code review are good. Fogel’s guide has a section titled "Practice Conspicuous Code Review". I think it serves a wide range of goals by being conspicuous.
Getting the mind set for OU into a positive place and keeping it there falls to the leaders in the group. Here JW and Mac set the foundation attitudes. Those supporting them provide some personal spin to the attitudes, but that is like planets orbiting a sun. They never get too far away from the core without leaving the group. So, I think OU has a good foundation.
Where I depart from Martin is at his statement, "When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group."
Respect, trust, transparency… they create a good environment. They do not exclude 'good intentions' gone bad. Going a bit deeper into the Emerald scandal for an example, several of the developers on the Emerald Team were creating content for SL. That content sold for RL money and was being stolen. They saw a problem.
Members of the team began to look for a way to stop the thieves taking their money/stuff. That grew into the Onyx Project and then that grew into other protection and tracking tools. They filled a need. Then people from outside the group figured out what was going on and took advantage of the code within the Emerald viewer. The result was Emerald users were at risk because of code that made it into the viewer for reasons thought to be good.
I’m not discounting Marten’s points on the importance of attitude, respect, trust, and transparency. But, they are not an absolute or even a good defense. They lead to the Emerald Teams's downfall. The people that actually placed the code into viewer did so for what some consider noble reasons.
In our community we have idealistic arguments over Drizzle. People see it differently. People presented with the same scenario come to different positions. For this… reason… attitude, respect, and trust are not impregnable defenses nor particularly good ones. Great for motivation and creativity and teamwork, but not so much for defense.
Putting all the esoteric reasoning aside, things can be simple. Review the code because it is vulnerable to abuse and mistakes. Not doing so would be disregarding the safety of those using the product. Keep everything transparent and be aware that at any moment someone may have a misguided idea and act on it and more likely just make a mistake.
There are people that constantly look for ways to get passwords. As small as Uru is, it is still a source of access to people’s computers. We will have people coming in who may have hidden agendas. Still we want to welcome and trust new people. But, verify their code.
Rarified’s goals for code review are good. Fogel’s guide has a section titled "Practice Conspicuous Code Review". I think it serves a wide range of goals by being conspicuous.
- It isn’t personal, it is a standard. We all know that.
- It isn’t just security, it is quality and security; motivation and method for good and a deterrent to abuse. We care about the user experience.
- Quality is important. This isn’t just fun programming, it is service to community and project too.
- We make mistakes. Help me find mine and I'll help others find theirs.
Nalates
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
Re: Getting the Ball Rolling
Good discussion. If i understand correctly, Nalates is saying that even with a good environment, good intentions can result in trouble. But, I counter, at the point the good intentions went awry, there was no longer a good environment!
Let's look again at the line of mine that was quoted, but with a different emphasis.
Anyway, I'm loving the the information from Fogel. The bullet points that Nalates highlighted are excellent, especially that first one. It isn't personal. (It isn't babysitting!)
Let's look again at the line of mine that was quoted, but with a different emphasis.
Now, I know Nalates covered this situation in detail when it happened, on her blog, so I turned there for more information. http://blog.nalates.net/2010/08/15/emer ... %a6-again/ gives me an error when I try to access it, but I found this quote at http://ambrosiadanceclub.com/random-ran ... ld-viewer/, with emphasis mine:Marten wrote:When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group.
That is the antithesis of "open and sharing." If the code and its purpose had been open for review from day one, and the contributors had not been secretive but rather had been sharing in their knowledge and concerns, then events would have unfolded differently. The consequence of this event is that our perspectives are adjusted from the illusion to reality... the environment of Emerald Viewer's development wasn't one of openness and sharing.Nalates wrote:After another round of complaints the data is now apparently encrypted making it extremely difficult to see what is being sent. I have to ask why, when caught, they went to XOR and, when caught again, then went to encryption? I find this behavior very indicative of a problem. Not only is the code closed so is the data being sent.
Anyway, I'm loving the the information from Fogel. The bullet points that Nalates highlighted are excellent, especially that first one. It isn't personal. (It isn't babysitting!)
The music is reversible, but time is not.
Re: Getting the Ball Rolling
Here is the repaired link to the Emerald article. http://blog.nalates.net/2010/08/15/emer ... pts-again/ When I moved the blog the extended characters (in this case … in place of ...) that were originally handled in the old blog fail in the new one. I’m slowly finding them. I never realized how many extended characters Word inserts. I should build a 404 trap to notify me that a page failed…
The Emerald Team started out as you describe, with a good environment and intentions. Things went bad along the way and by the time of the quote you use they were a shattered team in siege mentality.
@Martin, if you’re saying that a good environment is all we need to protect fans from Trojans or other misguided programmers, we disagree. If you are saying with a good environment bad apples won’t come in or good people won’t have different ideas and go counter to the team goals for what they consider good reasons, we disagree. We know we have black and gray hats in the community already. Even small close nit rural communities where people know everyone they still have law enforcement. Judas was trusted.
I simplify believe that good code review will do the most to achieve the safest and most error free code possible. If you think we should not use code review, we disagree. Readers can decide whether my thinking or your thinking is more realistic in this regard. I think the consensus so far is that we do want code review.
I’m not minimizing the value of a good environment. I think OU has a good environment now.
I’m not sure what we could change to improve it. Maybe stating code review is part of life at OU, or at least the goal. Adding stated reasons for why OU members want code review would avoid speculation and counter any that may misrepresent OU’s motives.
The Emerald Team started out as you describe, with a good environment and intentions. Things went bad along the way and by the time of the quote you use they were a shattered team in siege mentality.
@Martin, if you’re saying that a good environment is all we need to protect fans from Trojans or other misguided programmers, we disagree. If you are saying with a good environment bad apples won’t come in or good people won’t have different ideas and go counter to the team goals for what they consider good reasons, we disagree. We know we have black and gray hats in the community already. Even small close nit rural communities where people know everyone they still have law enforcement. Judas was trusted.
I simplify believe that good code review will do the most to achieve the safest and most error free code possible. If you think we should not use code review, we disagree. Readers can decide whether my thinking or your thinking is more realistic in this regard. I think the consensus so far is that we do want code review.
I’m not minimizing the value of a good environment. I think OU has a good environment now.
I’m not sure what we could change to improve it. Maybe stating code review is part of life at OU, or at least the goal. Adding stated reasons for why OU members want code review would avoid speculation and counter any that may misrepresent OU’s motives.
Nalates
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
GoW, GoMa and GoA apprentice - Guildmaster GoC - SL = Nalates Urriah
Re: Getting the Ball Rolling
I think you're misunderstanding Marten about support for reviews, but I'm sure he'll let you know. 
Personally, I prefer to have my code reviewed. If it's good, it's great to know people read it, say it's good, and maybe that they even learned something. If it's bad, what I would most appreciate hearing is the better way to do it rather than just hearing it's bad. I don't learn anything from "Bad code, bad." I do enjoy learning from people who can do it better and show me how. They are the kind of people I tend to gravitate toward.
Personally, I prefer to have my code reviewed. If it's good, it's great to know people read it, say it's good, and maybe that they even learned something. If it's bad, what I would most appreciate hearing is the better way to do it rather than just hearing it's bad. I don't learn anything from "Bad code, bad." I do enjoy learning from people who can do it better and show me how. They are the kind of people I tend to gravitate toward.
Perfect speed is being there.
Re: Getting the Ball Rolling
Nalates, we both want code reviews. In that, our interests are aligned. We're almost saying the same thing... but in this case what matters to me is not what we're saying, but how we're saying it.Nalates wrote: @Martin, if you’re saying that a good environment is all we need to protect fans from Trojans or other misguided programmers, we disagree. If you are saying with a good environment bad apples won’t come in or good people won’t have different ideas and go counter to the team goals for what they consider good reasons, we disagree. We know we have black and gray hats in the community already. Even small close nit rural communities where people know everyone they still have law enforcement. Judas was trusted.
Your reasoning has thus far been "We must have code reviews because we cannot trust people. Bad things happen if we don't have code reviews." Do you see how that is negative?
My reasoning is that "We must have code reviews to build increased trust. Good things happen when we have code reviews." Do you see how that sends a different message than what you've been saying?
Back to the statement I made about the Emerald Viewer (and this is my final attempt to explain this in the forum)...
Openness, trust, and sharing are two-way streets. Earlier, you said "Judas was trusted." That's great, but it didn't matter because Judas wasn't equally trusting in return. And in the case of the Emerald Viewer, It doesn't matter that others trusted the emkdu developers. Trust in one direction only does not constitute the "good" environment of openness and trust that I've been preaching about. The developers of the emkdu library closed its source code. To do so, they had to stop being open, trusting, and sharing in their knowledge! So, my statement is correct.Marten wrote:When people have respect for each other and are open, trusting, and sharing in their knowledge... there's no place for the sort of hijinks that befell the Emerald Viewer group.
The music is reversible, but time is not.