Code review malediction
-
Time for another story from a certain Korean corporation.
A certain abomination of an OS with a capital T in its name, keeps all its sources in git. The integration is so tight, that components are built by developers using a tool named Git Build System. For a long time development was based on git repos with gerrit in between for code review. But something sinister lurked in the shadows and one day decided to strike.
In all their wisdom, management decided that developing a Linux-based OS should be done on Windows; all builds shouldn’t be done locally but sent to some build server and the results downloaded, and that became the Standard. Teams in Korea began working with Visual Studio, while other teams in the rest of the world managed to keep their old ways and actually stay productive.
At some point, management decided that all issue tracking software in the world is not enough and decided to devote a metric ton of money and countless manhours to build something, which managed to reshape the definition of “broken beyond uselessness”. A form so chaotic, it’s beautiful in a perverse way. A single issue is made of countless tabs of forms, each spanning more than a FullHD monitor can show. All written in 1/3 English, 1/3 Korean and 1/3 unknown hybrid language, with features like -13 456 bug count and issue state diagrams taking three Excel sheets to describe (bonus WTF: drawing state diagrams in Excel). Thus the Standard was amended.
What does all that have to do with git and developer environment? In time Windows guys with Korean management (which has the final say in everything without discussion) discovered that git on Windows is somehow even worse than git in general, and their workflow generally sucks. They also discovered an ancient proprietary VCS called Perforce which worked on Windows and could be integrated with the issue tracker. Therefore the only logical conclusion was to start using Perforce. They only forgot to tell everyone else to stop using git…
Time went by and Bad Things started to appear. Git/gerrit was official in some teams, but Perforce was official in other teams (even working on the same component). Some patches went there, some there. The management finally decided Perforce code should be used as THE source for building OS images. Again, they only forgot to tell everyone else to stop using git…
Both repositories diverged to the point of being almost incompatible. Issues in Perforce code were given to git teams, which resulted in a litany of WTFs. After all, there’s not many things more fun than being tasked with fixing a bug in code that you physically don’t have. ASAP. Meetings took place, arrangements were made to rectify the situation. Months later, the situation is still the same.
One implication was code review process. With gerrit in place, that was a non-issue. But the Korean teams didn’t (and still don’t) understand the notion of code review and pushed everything directly to the repo. The quality of some patches was so bad that enforcing code review became top priority for non-Korean teams. Finally, a solution was developed – MS Word based code review. Each changeset needs to be attached to a bug in the tracker. Each bug can have a Word document attached with a request for code review. That document is a three pages long form with information so useless, nobody even wants to read it. At the end there’s a place for copy-pasting a diff for each file changed, with the explanation why. Reviewers are supposed to fill a Word form with details about which line they comment on and what their issue with it is.
Submitting a patch, clicking through the awful issue tracker and filling the form takes literal hours. All this because using git with gerrit was too tough. Fortunately, the review form has fields listing times taken by various steps in fixing a bug. Maybe someday someone will read how long pushing the code actually takes.
No, they won’t.
-
A certain abomination of an OS with a capital T in its name,
Tandem NonStop?
-
Jesus.
Are you staying with Samsung? You sound like a competent programmer from your other posts on this debacle, and I wouldn't want to touch Tizen with a 100 foot pole after all you've said about it.
-
Currently I'm at a point where I'm making something that's actually good and I'm my own boss about it. After this is finished, I don't know.
-
Hey, don't judge superior Korean technology(TM). Who doesn't love all the crapware and horrible UI shoved onto Android to justify the existence of managers and teams who otherwise wouldn't be employed?
-
In all their wisdom, management decided that developing a Linux-based OS should be done on Windows
Two sentences in and I can't in good faith say this is a non-front page WTF. Please use the submission form.
-
TRWTF is you didn't integrate git with perforce. Actually, no, I can't even say that with a straight face. It probably would have helped a little though. I'm assuming here that you can check changes into a git repo using Perforce's tools.
Based on what I've been told and have read Perforce is really good with tracking and dealing with large binary files so it's used where those types of files need to be tracked (read: art/game assets)
-
TRWTF is you didn't integrate git with perforce. Actually, no, I can't even say that with a straight face.
Based on what I've been told and have read Perforce is really good with tracking and dealing with large binary files so it's used where those types of files need to be tracked (read: art/game assets)
Ah look at that, a "Look at what people are talking" box also known as companies I will never work for.
Guess Trend Micro and vmware are off that list.
-
I haven't used Perforce, so maybe everyone else sees it as a giant WTF but being able to host all of your binary and non-binary assets in one repo without making one of them horribly slow sounds good.
My straight face joke was saying that wasn't The Actual Real WTF. There are much more TRWTF worthy things in this article.
-
OMG! Front page article, except for the free publicity which might cost you your job.
Now that I think about it, I've never used any Sammy software that didn't suck. I still have nightmares about some sync software for the GalaxyS which was Windows only.
Isn't Tizen open source? They shouldn't expect a lot of voluntary contribution using that Word (ugh!!!!) form. I would kill if I was forced to do this sort of crap... Diffs in Word... Amazing.
-
TRWTF is you didn't integrate git with perforce.
That's a part of the story I left out. Actually we tried that (and we're still trying to push that), but the Korean side doesn't want this. To understand the reasons, you have to understand how Korean management works - a Korean guy says "no" and that's it (in our corp being Korean means you are above non-Koreans and only your word matters). There's no discussion, only obedience. All your arguments become meaningless. It's really, really hard to push any kind of change to the process.
-
I use a phone made by this company and it is really good.
How do they produce competent hardware with such a flawed management/decision process?
It would seem hardware development is much less forgiving than software.
-
How do they produce competent hardware with such a flawed management/decision process? It would seem hardware development is much less forgiving than software.
That story will also be told...
-
TRWTF is you didn't integrate git with perforce.
Pshaw! Think BIG. Integrate with Discourse!
-
Should I expect the phone to explode soon?
-
explode
asplode
assplode
buttplode
buttplode.
Should I expect the phone to buttplode soon?
snickers
-
Stroll around the forums of cyanogen and the only difference with WTDWTF is Discourse.
-
That story will also be told...
Now I really have to ask that friend of mine if he's till enamored with Tizen. He was waxing almost poetically about it last time we met. That was more than two years ago, though.
-
-
I remember when Tizen was supposed to be the Android killer.
And I believe Google actually sacrificed Motorola to get Samsung to agree not to use it for phones.
They should have just let Samsung burn in MS Office Bug Tracking hell.
-
I remember when Tizen was supposed to be the Android killer.
The only way they'll make an Android killer is if they make the Android developers laugh themselves to death.
-
Based on what I've been told and have read Perforce is really good with tracking and dealing with large binary files so it's used where those types of files need to be tracked (read: art/game assets)
We use it for our documentation and specs (Word, Excel). Our source code, at least the parts of it I touch, are in a system that is basically Perforce with a thin, domain-specific wrapper around it. (It is not at all clear to me what functionality the wrapper adds, other than getting it past someone, some time in the past, who said "NO PERFORCE!" for some reason.) In any event, it works well for us.
-
How do they produce competent hardware with such a flawed management/decision process?
I wonder that about my employer, too.
-
A recent example why code review is good. Let me lay out logic for a patch pushed by our Korean friends:
"if the (required) launched application identifier is not YouTube and the length of the id is more than 0, kill it"
Can't wait for the OS image with this change.
-
Did I understand that right? It kills everything except YouTube?
-
Fortunately, the review form has fields listing times taken by various steps in fixing a bug. Maybe someday someone will read how long pushing the code actually takes.
No, they won’t.
Submit unacceptable push times as a bug report on the issue tracker, via the issue tracker.
Filed under: recursive bug reports: good enough for dicksauce, good enough for you
-
Bug: Users can no longer launch the dialer, only YouTube.
Solution: Allow launching the dialer.would be vetoed by Korean management as obviously their code is perfect in every way.Solution: Evangelize Google to include Google Voice as a core YouTube feature.
-
Launching the dialer is a barrier to Grumpy Cat.
-
Despite having read this topic several times previously, my brain just parsed the title1 as "Code review medication."
E_NO_CAFFEINE
1 Actual title at the time of posting, since titles are subject to rapid and random change: Code review malediction
-
Pretty much although the conditional for that didn't fit the whole screen horizontally, so it was probably an honest mistake, if you can call such abomination honest. Also that should give you an idea about code testing.
Side note - youtube is a special snowflake on certain class of devices. Tizen is so bad, that to make youtube work, special hacks had to be made in the kernel just for this application.
-
I'm a bit mystified as to why you would want a conditional of this sort leading to a
kill taskin the first place?
-
Don't know; didn't ask; pretty sure I don't want to know.
-
youtube
special hacks had to be made in the kernel
Excuse me, I need to pick up bits of my brain from the floor...
-
Before you put them back in your head, remember that you're living in a world where the dominant commercial OS has an in-kernel HTTP server.
-
Can we run Discourse on it?
-
No. It only serves static pages. But it does that really really fast.
-
Can we run Discourse on it?
A discourse kernel module? Excellent idea.
Filed under: posting this caused 4
blue screenserror 500
-
If you're only serving static content, it's really easy to write a web server. A handful of lines of code (in a sensible programming language, or rather a lot more in C). Most of the time is going to be just doing a disk-to-network-card direct copy, which takes virtually no CPU at all even on poor systems. Ones where you can DMA the content directly are even better.
-
If you're only serving static content, it's really easy to write a web server.
Hell, I did. Ok, it's an extremely basic one and I should make it return more than 404s and 200s only (403s would make actual sense for what it does, too, but I prefer 200 with error content for this purpose), but yeah, took like 40-50 lines of code in Qt?
-
Hell, I did. Ok, it's an extremely basic one and I should make it return more than 404s and 200s only (403s would make actual sense for what it does, too, but I prefer 200 with error content for this purpose), but yeah, took like 40-50 lines of code in Qt?
Did it for a systems programming course in plain C (but with POSIX socket stuff). It was rather more than 50 lines (a few hundred), but still quite doable. We did it event based; I think a
forking version would have been more compact.In that project I also found out that it's impossible to write a function in C that returns a pointer to itself, at least without any casts.
-
Ones where you can DMA the content directly are even better.
IIRC the point of the Windows in-kernel HTTP server was to facilitate the interoperation of typical NIC scatter-gather DMA with disc controller DMA to achieve zero-copy HTTP response generation; the thing includes system calls to allow IIS to take advantage of that, apart from being a static-page HTTP server in its own right.
It's not a bad idea if you're after dragging as much performance out of the hardware as HTTP will allow, but jesus christ the attack surface! You'd want to code review the shit out of that thing.
-
but jesus christ the attack surface! You'd want to code review the shit out of that thing.
That's why the in-kernel HTTP server had better be extremely simple, so that it is possible to identify every possible code path and ensure that every last one does what it is supposed to. So yes, serious code review required, and anything where you can't be entirely sure it's correct has to go to user code. There are software engineering practices that can give that level of assurance, but they're usually not used as they're really expensive; this would be a case where they're justifiable.
-
There are software engineering practices that can give that level of assurance, but they're usually not used as they're really expensive; this would be a case where they're justifiable.
More details please? Would it just be something like "get your 5 best engineers to review every line very slowly" or something more complicated?
-
I also found out that it's impossible to write a function in C that returns a pointer to itself, at least without any casts.
Sure you can. Using it is a little clumsy because you have to wrap the pointer in a figleaf of struct syntax, just to overcome the fact that function declarations need a predefined return type but recursive
typedefisn't allowed:stephen@kitchen:/tmp$ cat ptr-to-self.c #include <stdio.h> struct wrapped_pf { struct wrapped_pf (*p)(); }; struct wrapped_pf my_own_address() { struct wrapped_pf wp = {&my_own_address}; return wp; } int main(int argc, char *argv[]) { printf("%d\n", my_own_address().p == &my_own_address); return 0; } stephen@kitchen:/tmp$ gcc -O3 -Wall ptr-to-self.c stephen@kitchen:/tmp$ ./a.out 1A snippet of the output from
gcc -O3 -S ptr-to-self.cshows clearly that a struct with a single member is indeed nothing more than a syntactical figleaf:.globl my_own_address .type my_own_address, @function my_own_address: .LFB11: .cfi_startproc movl $my_own_address, %eax ret .cfi_endproc
-
Would it just be something like "get your 5 best engineers to review every line very slowly" or something more complicated?
More complicated. You're really talking about using things like VDM (or one of the less well known variations) and you're having to start out with what might be best described as a total formal description of what the code does in all cases. It's very math and proof heavy, and is usually only used for safety-critical applications like railway signalling. The cost of failure is pretty high with an in-kernel HTTP server in a widely-deployed platform too.
The last time I heard about costs of this sort of thing, they were estimated to be somewhere between ⅓ and ½ of the cost of getting 100% test coverage by normal techniques. Health warning: those figures were from well over a decade ago, so maybe the cost of testing has reduced?
-
-
clearly that a struct with a single non-virtual member is indeed nothing more than a syntactical figleaf:
fixed
-
a trivially copyable struct with a single member and standard layout and trivial default constructor is indeed nothing more than a syntactical figleaf:
fixed^2
-
This is C! We don't "constructor" here!
-
