Closed
Bug 458950
Opened 16 years ago
Closed 15 years ago
updater doesn't build for windows CE
Categories
(Toolkit :: Application Update, defect, P1)
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)
32.55 KB,
patch
|
benjamin
:
review-
|
Details | Diff | Splinter Review |
17.68 KB,
patch
|
benjamin
:
review+
vlad
:
review+
|
Details | Diff | Splinter Review |
17.77 KB,
application/zip
|
Details | |
2.38 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•16 years ago
|
||
this gets wince building, but breaks my build for winxp... definitely needs more work.
Assignee: nobody → blassey
Status: NEW → ASSIGNED
Assignee | ||
Updated•15 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•15 years ago
|
OS: Windows XP → Windows CE
Hardware: x86 → ARM
Assignee | ||
Comment 2•15 years ago
|
||
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)
Comment 4•15 years ago
|
||
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, >+ ®Key, >+ 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-
Updated•15 years ago
|
tracking-fennec: ? → 1.0b1-wm+
Assignee | ||
Comment 5•15 years ago
|
||
(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?
Comment 6•15 years ago
|
||
There are basic tests in the tree for applying a partial and a complete mar file. http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/test/unit/test_0110_general.js
Assignee | ||
Comment 7•15 years ago
|
||
marking this bug as dependent on being able to run unit tests
Comment 9•15 years ago
|
||
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.
Assignee | ||
Comment 10•15 years ago
|
||
this patch also passes xpcshell tests on windows desktop, I've sent it to try server to check linux and osx.
Assignee | ||
Updated•15 years ago
|
Whiteboard: [nv]
Assignee | ||
Updated•15 years ago
|
Attachment #383658 -
Flags: review?(benjamin)
Assignee | ||
Comment 11•15 years ago
|
||
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.
Comment 12•15 years ago
|
||
log files after running xpcshell tests on htc touch pro
Comment 13•15 years ago
|
||
(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
Priority: -- → P1
Comment 14•15 years ago
|
||
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?
Comment 15•15 years ago
|
||
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.
Comment 16•15 years ago
|
||
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?
Assignee | ||
Comment 17•15 years ago
|
||
(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.
Assignee | ||
Comment 18•15 years ago
|
||
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.
Comment 19•15 years ago
|
||
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 20•15 years ago
|
||
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?
Comment 22•15 years ago
|
||
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+
Updated•15 years ago
|
Attachment #383658 -
Flags: review?(benjamin) → review+
Comment 24•15 years ago
|
||
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
Comment 25•15 years ago
|
||
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.
Assignee | ||
Comment 26•15 years ago
|
||
pushed http://hg.mozilla.org/mozilla-central/rev/bb35d6826f1d and filed bug 504683 for disabling LaunchWinPostProcess for windows ce
Comment 27•15 years ago
|
||
Attachment #389219 -
Flags: review?(bugmail)
Comment 28•15 years ago
|
||
There is still at least one other bug - bug 504432 - that prevents using the updater on WinCE and probabally WinMo.
Assignee | ||
Updated•15 years ago
|
Attachment #389219 -
Flags: review?(bugmail) → review+
You need to log in
before you can comment on or make changes to this bug.
Description
•