Closed Bug 458950 Opened 12 years ago Closed 11 years ago

updater doesn't build for windows CE

Categories

(Toolkit :: Application Update, defect, P1)

ARM
Windows CE
defect

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 1.0b1-wm+ ---

People

(Reporter: blassey, Assigned: blassey)

References

Details

(Keywords: mobile, Whiteboard: [nv])

Attachments

(4 files, 1 obsolete file)

No description provided.
Attached patch WIP patch (obsolete) — Splinter Review
this gets wince building, but breaks my build for winxp... definitely needs more work.
Assignee: nobody → blassey
Status: NEW → ASSIGNED
tracking-fennec: --- → ?
OS: Windows XP → Windows CE
Hardware: x86 → ARM
Depends on: 479382
Benjamin, this gets us building on wince and resolves the issue with building on windows from the previous patch.  I'm asking for your review to see if I'm going in the right direction here. 

Also, what is a good way to test this code?
Attachment #342136 - Attachment is obsolete: true
Attachment #363643 - Flags: review?(benjamin)
Duplicate of this bug: 430686
Comment on attachment 363643 [details] [diff] [review]
gets us building on wince

>diff --git a/toolkit/mozapps/update/src/updater/Makefile.in b/toolkit/mozapps/update/src/updater

>+ifdef WINCE
>+# Pick up nsWindowsRestart.cpp
>+LOCAL_INCLUDES += -I$(topsrcdir)/toolkit/xre
>+REQUIRES += string
>+RCFLAGS += -D_WIN32_WCE=600 -DWINCE -I$(topsrcdir)/build/wince/shunt/include/ -d"_DEBUG" -d"_UNICODE" -d"UNICODE" -d"_WIN32_WCE" -d"UNDER_CE" -l 0x409 
> CXXFLAGS += $(BZ2_CFLAGS)
>+endif

Hrm, is there a more generic place all these RCFLAGS can go? It seems odd to be putting generic flags here, instead of in config.mk or someplace.

>\ No newline at end of file

nit, please fix

>diff --git a/toolkit/mozapps/update/src/updater/bspatch.cpp b/toolkit/mozapps/update/src/updater/bspatch.cpp

>-int
>-MBS_ApplyPatch(const MBSPatchHeader *header, int patchfd,
>-               unsigned char *fbuffer, int filefd)
>+
>+#ifdef WINCE
>+int MBS_ApplyPatch(const MBSPatchHeader *header, FILE* patchf,
>+           unsigned char *fbuffer, FILE* filef)
>+#else
>+int MBS_ApplyPatch(const MBSPatchHeader *header,int patchfd,
>+                   unsigned char *fbuffer, int filefd)
>+#endif

Why fork this signature? If you're going to use FILE*, I think you might as well use it on all platforms. The only concern is if you have mixed C runtimes, you can't pass a FILE* across the boundary: but I'm pretty sure this patch code only gets linked into executables, never into shared libraries, so that's not a concern.

>diff --git a/toolkit/mozapps/update/src/updater/updater.cpp b/toolkit/mozapps/update/src/updater/updater.cpp

>+#ifdef WINCE
>+      PRUnichar wide_path[MAX_PATH];
>+      MultiByteToWideChar(CP_ACP, 0, path, -1, wide_path, MAX_PATH);
>+      CreateDirectory(wide_path, NULL);
>+#else
>       rv = mkdir(path, 0755);
>+#endif

Patterns like this seem like they could/should be refactored into a little helper function, rather than scattered throughout.

>+  FILE* sfd = fopen(spath, "RB");
>+
>+#ifdef WINCE
>+  if (sfd <= 0) {
>+#else
>   struct stat ss;
>-
>-  AutoFD sfd = open(spath, O_RDONLY | _O_BINARY);
>-  if (sfd < 0 || fstat(sfd, &ss)) {
>+  if (sfd < 0 || fstat(fileno(sfd), &ss)) {
>+#endif

Why aren't you using AutoFD for WINCE? Don't you have to close the file just like other OSes? Or is this more FILE/fd differences? If so, just use FILE* throughout.

>+ inline int _waccess(const PRUnichar* wide_path, int mode) {

>+ inline int access(const char* path, int mode) {

make both of these static



> int
> PatchFile::Execute()
> {
>   LOG(("EXECUTE PATCH %s\n", mFile));
> 
>   // Create backup copy of the destination file before proceeding.
> 
>+#ifndef WINCE
>   struct stat ss;
>   if (stat(mFile, &ss))
>     return READ_ERROR;
>+#endif

Shouldn't WINCE check for file existence? Or is that covered somewhere else?

>+#ifdef WINCE
>+  DWORD exefileLen = MAXPATHLEN;
>+  DWORD exeargLen = MAXPATHLEN;
>+  
>+  HKEY regKey;
>+  if (RegCreateKeyExW ( HKEY_LOCAL_MACHINE, 
>+            PROFILE_REG_KEY L"PostUpdateWin", 
>+            0,
>+            NULL, 
>+            REG_OPTION_NON_VOLATILE, 
>+            KEY_READ, 
>+            NULL, 
>+            &regKey, 
>+            NULL) != ERROR_SUCCESS)

I don't think these registry calls make sense: they were reading from an application INI file. Now they're reading from the registry? How did that data get into the registry to begin with?

>+#ifndef WINCE // XXX need to fix

This is a pretty big deal. It's obvious that updater isn't going to work at all... so why are you patching it? Why not just avoid building it (make --disable-updater the default for WINCE)

robstrong should have a look at this either way.
Attachment #363643 - Flags: review?(benjamin) → review-
tracking-fennec: ? → 1.0b1-wm+
(In reply to comment #4)

> >+#ifndef WINCE // XXX need to fix
> 
> This is a pretty big deal. It's obvious that updater isn't going to work at
> all... so why are you patching it? Why not just avoid building it (make
> --disable-updater the default for WINCE)


Right, this is a WIP but I wanted to get a review from you to see if I was missing any big gotchas.  Thanks for taking the time.

Do you know of any good ways to test this code?
marking this bug as dependent on being able to run unit tests
Depends on: 477597, 482085
adding dependency on Fennec providing an updater.ini
Depends on: 477594
btw: the test_0110_general.js and test_0111_general.js have been rewritten so they no longer require the update service and are now very simple. You could also just run the updater binary with the command line args that are used in the test to apply the test mar files.
this patch also passes xpcshell tests on windows desktop, I've sent it to try server to check linux and osx.
Depends on: 498394, 498844, 498845
Depends on: 499184
Attachment #383658 - Flags: review?(benjamin)
Comment on attachment 383658 [details] [diff] [review]
gets us building on windows ce

Joel has the unit tests running locally and tells me that all of the updater tests passed with this patch.
log files after running xpcshell tests on htc touch pro
(In reply to comment #12)
> Created an attachment (id=387361) [details]
> set of logs for xpcshell tests
> 
> log files after running xpcshell tests on htc touch pro
Looks like several of the files needed to run the tests aren't present which caused several of the tests to fail including the updater tests
rs, thanks for pointing out the missing files issue.  I did notice that as well and saw that our workaround for get_cwd() (adding it via --environ:CWD=<blah>) works I just had the wrong value in there.

Of course these tests are very fragile on windows mobile and I have seen them all pass, just not at once.

This makes me wonder why I was getting TEST-PASS status when there were obvious failures.  Should we check every little thing in each test to verify that it deserves a TEST-PASS?
Hey Joel, the three tests that failed are test_0030_general.js, test_0110_general.js, and test_0111_general.js and they all have TEST-UNEXPECTED-FAIL in the logs so it isn't clear to me why you would count them as TEST_PASS.
Brad/bsmedberg:
 
We're looking to generate nightly updates as part of build automation on WinCE, so from reading comment#4, comment#5, not sure if this is the right bug to be blocked on.

Can you confirm if this bug is about getting updates working on WinCE and if so, any ETA on this?
(In reply to comment #16) 
> Can you confirm if this bug is about getting updates working on WinCE and if
> so, any ETA on this?

This bug is for getting the updater working for wince. I'd like to get this in for fennec alpha 3, which is scheduled for next week.
with the patch on bug 503137, which allows xpcshell to handle our cwd "system", all tests pass except for test_0030_general.js, which hangs

https://wiki.mozilla.org/User:Blassey/UpdaterTests#test_0030_general.js_.5BHANG.5D

Rob tells me that this is the only test that relies on nsHttpServer.  I'm not sure if that's enough of an excuse to ignore the hang or not.
I filed bug 503409 for the test_0030_general.js hang. I'm going to make it throw if the directory specified for nsHttpServer registerDirectory doesn't exist or isn't a direcory in that bug in case that is the cause.
Comment on attachment 383658 [details] [diff] [review]
gets us building on windows ce

Nit: wince_updater.{c,h} have CRLF line endings.
Given comment #18, is there anything else blocking this from landing?
Joel verified that test_0030_general.js passes using an external web server so it appears that nsHttpServer isn't working properly on Windows CE.
Comment on attachment 383658 [details] [diff] [review]
gets us building on windows ce

>@@ -763,8 +769,8 @@ int PatchFile::sPatchIndex = 0;
> 
> PatchFile::~PatchFile()
> {
>-  if (pfd >= 0)
>-    close(pfd);
>+  if (pfile >= 0)
>+    fclose(pfile);

just if (pfile)

Looks fine otherwise, except updater_wince.{cpp,h} need a license block.
Attachment #383658 - Flags: review+
Attachment #383658 - Flags: review?(benjamin) → review+
Comment on attachment 383658 [details] [diff] [review]
gets us building on windows ce

>+#ifdef WINCE
>+  //XXX need to use registry

You definitely do not want to use the registry. Maybe use nsINIParser instead of GetPrivateProfileString if that function isn't available on WINCE.

>diff --git a/toolkit/mozapps/update/src/updater/updater_wince.cpp b/toolkit/mozapps/update/src/updater/updater_wince.cpp

>+int remove(const char* path) {

nit, the opening brace of this and other functions below should be on the following line.

>+  if (!_unlink(path)) {
>+    return 0;
>+  } else if (GetLastError() == ERROR_ACCESS_DENIED) {

nit, put the `else` on the following line here and below
Until nsINIParser or code similar to readstrings.cpp is used I suggest ifdefing out LaunchWinPostProcess entirely since it isn't clear that post update work will need to be done on Windows Mobile / CE. Also, you shouldn't need the lock file code since I don't believe there is an option to launch with the runas verb on Windows Mobile / CE.
Blocks: 504683
pushed http://hg.mozilla.org/mozilla-central/rev/bb35d6826f1d

and filed bug 504683 for disabling LaunchWinPostProcess for windows ce
No longer blocks: 504683
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Depends on: 504917
There is still at least one other bug - bug 504432 - that prevents using the updater on WinCE and probabally WinMo.
Attachment #389219 - Flags: review?(bugmail) → review+
Depends on: 506079
You need to log in before you can comment on or make changes to this bug.