Closed Bug 482255 Opened 11 years ago Closed 11 years ago

need remote test harness for windows ce

Categories

(Firefox Build System :: General, defect)

ARM
Windows CE
defect
Not set

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: blassey, Assigned: blassey)

Details

(Keywords: mobile)

Attachments

(1 file, 3 obsolete files)

We should be able to do this with CeCreateProcess passing in DEBUG_PROCESS to get a handle to stdout.  That's the plan anyway.
Flags: wanted-fennec1.0?
Attached patch host/client remote harness (obsolete) — Splinter Review
cestub.dll needs to be installed in the wince path (either \ or \Windows).  ceexec.exe takes the command line to execture (absolute path and args).
Attachment #366489 - Flags: review?(doug.turner)
Attached patch fixes returning exit code (obsolete) — Splinter Review
Turns out that IRAPIStream::Write won't actually write the data if less than 4 bytes is passed in.
Attachment #366489 - Attachment is obsolete: true
Attachment #366758 - Flags: review?(doug.turner)
Attachment #366489 - Flags: review?(doug.turner)
Comment on attachment 366758 [details] [diff] [review]
fixes returning exit code

nice try blassey.
Attachment #366758 - Flags: review?(doug.turner) → review-
Attached patch an actual patch (obsolete) — Splinter Review
oops, sorry

note though that the .def file gives warnings such as these:
c:/hg/mc-min/build/wince/remote/client/cestub.def(1) : warning LNK4017: # statement not supported for the target platform; ignored

That is for the license block.  Ignoring is what we want it to do, but its noisy.
Attachment #366758 - Attachment is obsolete: true
Attachment #366817 - Flags: review?(doug.turner)
(In reply to comment #4)
> note though that the .def file gives warnings such as these:
> c:/hg/mc-min/build/wince/remote/client/cestub.def(1) : warning LNK4017: #
> statement not supported for the target platform; ignored
> 
> That is for the license block.  Ignoring is what we want it to do, but its
> noisy.

You presumably can use semicolon(;) for comment line in def file.
Comment on attachment 366817 [details] [diff] [review]
an actual patch

+DIRS += wince/shunt \
+	wince/remote/client \
+	wince/remote/host \
+	$(NULL)


nit: These should be aligned.

+# Portions created by the Initial Developer are Copyright (C) 2___

Fix the year


+
+TOPSRCDIR = $(topsrcdir)
+OBJDIR = $(DEPTH)
+
+export NO_SHUNT = 1

Could you add a comment as to why you need these.

+ *   Brad Lassey <blassey@moilla.com>

missing a 'z'


+int  __cdecl RemoteExec(DWORD dwInput, BYTE* pInput,
+			DWORD* ret, BYTE** ppOutput,
+			IRAPIStream* pStream)


Align the params, and there are two spaces between int and __cdecl

ooc, do you need to explictly dllexport when using a def file?
+    space = wcschr(quote2, ' ');
+    
+  } else {

nit: Ditch the new line

+      switch(de.dwDebugEventCode) {
+      case EXCEPTION_DEBUG_EVENT:

nit: space the case statements in (they should be under the switch)


+  de.dwProcessId = pi.dwProcessId;
+  de.dwThreadId = pi.dwThreadId;


Does this matter?  I think WaitForDebugEvent fills these in, and you need to check, when WaitForDebugEvent returns from blocking, if the thread and process ID match what you expect.  Maybe for this app, you want to print every event, so just ditch the initial assignments.

+	  goto RETURN;

no religous debate here, but why not just have a keepGoing variable that we set, and have your for(;;) statement turn into while(keepGoing).


+	printf("process %x exited with code %d\n", de.dwProcessId, de.u.ExitProcess.dwExitCode);

Don't you want to print this line when de.dwProcessId does equal pi.dwProcessId

another general nit: fix the curly brace formating to be consistent.  Either under the statement above, on the same line, or under the statement above spaced in.  Pick one, stick to it, defend your choice relentlessly until your death.


+  char EOT[4] = {4, exitCode,0};  // We need the buffer to be big enough to get flushed

space between the comma and 0

+      length+= 3;
+    else
+      length+= 1;


needs comments.  1 is for the null, 3 is for the null, and ws.

+  WCHAR* args = (WCHAR*)LocalAlloc(LMEM_FIXED,length * sizeof(WCHAR));

Space after the comma.


+  args[length-1] = 0;

is this needed? since you are using *cat, shouldn't args always have a trailing null?

+    printf("failed to connecto to device\n");

mispelling


+	if (buf[i]  == 4) {

Weird spacing, and why 4?  comment needed.

can you just read one byte at a time and putchar it?  that will save you a bit of logic.


I skipped over OUTPUT_DEBUG_STRING_EVENT, but will review it when the rest is squared away.  I think my only major question there was answered on IRC.
Attachment #366817 - Flags: review?(doug.turner) → review-
Attached patch patch v.3Splinter Review
Attachment #366817 - Attachment is obsolete: true
Attachment #367845 - Flags: review?(doug.turner)
Attachment #367845 - Flags: review?(doug.turner) → review+
Aki isn't using these tools, so I'm not going to land them
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WONTFIX
Flags: wanted-fennec1.0? → wanted-fennec1.0-
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.