Closed Bug 330720 Opened 14 years ago Closed 12 years ago
Remove VACPP from NSPR
86.03 KB, patch
|Details | Diff | Splinter Review|
1.51 KB, patch
|Details | Diff | Splinter Review|
469 bytes, patch
|Details | Diff | Splinter Review|
This bug is to remove all VACPP references from NSPR and convert XP_OS2_EMX to XP_OS2
I updated the patch for bit-rot (which basically only happened in configure.in), against NSPR from Mozilla trunk. I have also looked through it and tested with a Firefox trunk build with GCC 3.3.5. Everything still works fine. The few other changes I made are - nsprpub/pr/include/prtypes.h: the previous patch basically doubled the section on OS2 && __declspec. I supposed that if GCC 3.2.2 builds worked fine before we don't need a section without __declspec at all. - As added to the end of the patch, nsprpub/pr/src/md/os2/os2vacpp.asm should be removed, too, as it is not necessary any more and was already removed from nsprpub/pr/src/md/os2/Makefile.in and nsprpub/pr/src/md/os2/objs.mk. - nsprpub/pr/src/io/prlog.c: Removed the extra comment at the top that is no longer relevant.
Comment on attachment 266989 [details] [diff] [review] Updated full diff Adding third review request. Best two out of three. :)
Attachment #266989 - Flags: review?(julien.pierre.boogz)
I tried to apply this patch but it didn't work for me. This seems to be a bug in the OS/2 port of the patch.exe tool. I was able to apply the patch on a Solaris tree, but can't test it there :(. This wasn't a problem with CR LF as I tried dos2unix and unix2dos and it wouldn't apply either way. Peter, can you point me to the patch.exe you are using successfully on OS/2 ?
Comment on attachment 266989 [details] [diff] [review] Updated full diff This patch looks all correct, even though I wasn't able to actually apply and test on my OS/2 machine due to a patch tool problem.
Attachment #266989 - Flags: review?(julien.pierre.boogz) → review+
Julien, you need to make sure that GNU patch and not the OS/2 patch (in bootdrive:\OS2) is first in the path. If that is the case it should work. The build requirements page lists http://hobbes.nmsu.edu/pub/os2/dev/util/gnupatch.zip and I believe that is what I am using...
Yeah, I had patch version 2.5 in my path, but for some reason it didn't apply the patches successfully to my fresh source tree. Does it depend on any other programs ?
(In reply to comment #7) > Does it depend on any other programs ? I don't think so.
Comment on attachment 266989 [details] [diff] [review] Updated full diff I don't think we need an extra OK from Mike as Julien already r+'ed this patch and the original one was made by Mike. Wan-Teh, do you still do NSPR superreviews? I guess an sr+ is still needed?
As I discovered while working on bug 399647, we should also remove RAMSEM when removing VACPP support. This patch does that, everything else is the same as in the older patch. (RAMSEM support was never implemented for GCC and we have been able to live without it in GCC builds for the past 4 years, so I don't think we will ever implement it. And I don't think there is nobody who could implement it...)
Comment on attachment 285000 [details] [diff] [review] Remove VACPP support and RAMSEM Nelson, perhaps you have time earlier than Wan-Teh to review this updated patch? If I need sr for NSPR, can you give that, too?
Comment on attachment 285000 [details] [diff] [review] Remove VACPP support and RAMSEM There's something wrong with this patch. It removes an entire file of assembler code, but the patch lacks some lines that are needed to properly signify which file and which revision is affected. That portion of the patch begins with these lines: >--- nsprpub/pr/src/md/os2/os2vacpp.asm 2006-01-09 18:00:12.000000000 +0000 >+++ nsprpub/pr/src/md/os2/os2vacpp.asm 2007-10-15 21:22:10.000000000 +0000 This is clearly not the output of cvs diff. The lines that begin that section should look like this: >Index: nsprpub/tools/Makefile.in >=================================================================== >RCS file: /cvsroot/mozilla/nsprpub/tools/Makefile.in,v >retrieving revision 1.12 >diff -u -8 -p -r1.12 Makefile.in >--- nsprpub/tools/Makefile.in 8 Nov 2004 02:52:56 -0000 1.12 >+++ nsprpub/tools/Makefile.in 15 Oct 2007 21:22:01 -0000 To get that output, do a cvs remove -fl nsprpub/pr/src/md/os2/os2vacpp.asm (which removes the file from your local workarea, but does NOT actually remove it from the repository). Then use cvs diff -Nu5 nsprpub/pr/src/md/os2/os2vacpp.asm to get the proper diff output. If you find that cvs will not let you do the remove command, then you can fake it. :) You can make the exact same change to the file mozilla/nsprpub/pr/src/md/os2/CVS/Entries that cvs remove would make if it succeeded. -/os2vacpp.asm/1.7/Mon Jan 9 17:43:52 2006// +/os2vacpp.asm/-1.7/dummy timestamp// You must also remove the file from your local workarea. Then the cvs diff -Nu5 command will give you the proper patch output which will begin with lines that look something like this: Index: nsprpub/pr/src/md/os2/os2vacpp.asm =================================================================== RCS file: nsprpub/pr/src/md/os2/os2vacpp.asm diff -N nsprpub/pr/src/md/os2/os2vacpp.asm --- nsprpub/pr/src/md/os2/os2vacpp.asm 9 Jan 2006 17:43:52 -0000 1.7 +++ /dev/null 1 Jan 1970 00:00:00 -0000 After making these changes, please ask Julien to review the new patch.
Wow, cool, thanks Nelson. I have used CVS for years but I have never discovered the -N switch for cvs diff (and nobody has ever complained about my local diffs with hacked headers for file removals/additions). I'm in the process of updating the patch.
OK, this is the updated patch against NSPR trunk. Differences against the last patch are the corrected header for os2vacpp.asm and that the diff for rules.mk is now included.
Btw, I don't see Julien listed as NSPR peer at <http://www.mozilla.org/owners.html#nspr>, should he be?
Comment on attachment 318221 [details] [diff] [review] Remove VACPP support and RAMSEM (with corrected os2vacpp.asm diff) This looks OK.
Attachment #318221 - Flags: review?(julien.pierre.boogz) → review+
Great, so I checked this into NSPR trunk. (Generating configure was a bit messy but I triple checked that it really only contained OS/2 changes in the end. Well, and different line numbers...) As this just removes unused code and has no functional changes, we don't need an extra tag or anything. Checkin log: Checking in nsprpub/configure; /cvsroot/mozilla/nsprpub/configure,v <-- configure new revision: 1.230; previous revision: 1.229 done Checking in nsprpub/configure.in; /cvsroot/mozilla/nsprpub/configure.in,v <-- configure.in new revision: 1.234; previous revision: 1.233 done Checking in nsprpub/config/Makefile.in; /cvsroot/mozilla/nsprpub/config/Makefile.in,v <-- Makefile.in new revision: 1.22; previous revision: 1.21 done Checking in nsprpub/config/now.c; /cvsroot/mozilla/nsprpub/config/now.c,v <-- now.c new revision: 3.13; previous revision: 3.12 done Checking in nsprpub/config/rules.mk; /cvsroot/mozilla/nsprpub/config/rules.mk,v <-- rules.mk new revision: 3.65; previous revision: 3.64 done Checking in nsprpub/lib/ds/Makefile.in; /cvsroot/mozilla/nsprpub/lib/ds/Makefile.in,v <-- Makefile.in new revision: 1.37; previous revision: 1.36 done Checking in nsprpub/lib/ds/plhash.h; /cvsroot/mozilla/nsprpub/lib/ds/plhash.h,v <-- plhash.h new revision: 3.10; previous revision: 3.9 done Checking in nsprpub/lib/libc/src/Makefile.in; /cvsroot/mozilla/nsprpub/lib/libc/src/Makefile.in,v <-- Makefile.in new revision: 1.33; previous revision: 1.32 done Checking in nsprpub/lib/msgc/tests/Makefile.in; /cvsroot/mozilla/nsprpub/lib/msgc/tests/Makefile.in,v <-- Makefile.in new revision: 1.14; previous revision: 1.13 done Checking in nsprpub/lib/prstreams/Makefile.in; /cvsroot/mozilla/nsprpub/lib/prstreams/Makefile.in,v <-- Makefile.in new revision: 1.23; previous revision: 1.22 done Checking in nsprpub/lib/prstreams/tests/testprstrm/Makefile.in; /cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/Makefile.in,v <-- Makefile.in new revision: 1.11; previous revision: 1.10 done Checking in nsprpub/lib/prstreams/tests/testprstrm/testprstrm.cpp; /cvsroot/mozilla/nsprpub/lib/prstreams/tests/testprstrm/testprstrm.cpp,v <-- testprstrm.cpp new revision: 3.7; previous revision: 3.6 done Checking in nsprpub/lib/tests/Makefile.in; /cvsroot/mozilla/nsprpub/lib/tests/Makefile.in,v <-- Makefile.in new revision: 1.18; previous revision: 1.17 done Checking in nsprpub/pr/include/prio.h; /cvsroot/mozilla/nsprpub/pr/include/prio.h,v <-- prio.h new revision: 3.41; previous revision: 3.40 done Checking in nsprpub/pr/include/prtypes.h; /cvsroot/mozilla/nsprpub/pr/include/prtypes.h,v <-- prtypes.h new revision: 3.36; previous revision: 3.35 done Checking in nsprpub/pr/include/md/_os2.h; /cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v <-- _os2.h new revision: 3.37; previous revision: 3.36 done Checking in nsprpub/pr/include/md/_os2_errors.h; /cvsroot/mozilla/nsprpub/pr/include/md/_os2_errors.h,v <-- _os2_errors.h new revision: 3.13; previous revision: 3.12 done Checking in nsprpub/pr/include/md/_pcos.h; /cvsroot/mozilla/nsprpub/pr/include/md/_pcos.h,v <-- _pcos.h new revision: 3.10; previous revision: 3.9 done Checking in nsprpub/pr/include/private/primpl.h; /cvsroot/mozilla/nsprpub/pr/include/private/primpl.h,v <-- primpl.h new revision: 3.89; previous revision: 3.88 done Checking in nsprpub/pr/src/Makefile.in; /cvsroot/mozilla/nsprpub/pr/src/Makefile.in,v <-- Makefile.in new revision: 1.47; previous revision: 1.46 done Checking in nsprpub/pr/src/cplus/tests/Makefile.in; /cvsroot/mozilla/nsprpub/pr/src/cplus/tests/Makefile.in,v <-- Makefile.in new revision: 1.11; previous revision: 1.10 done Checking in nsprpub/pr/src/io/Makefile.in; /cvsroot/mozilla/nsprpub/pr/src/io/Makefile.in,v <-- Makefile.in new revision: 1.14; previous revision: 1.13 done Checking in nsprpub/pr/src/io/prlog.c; /cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v <-- prlog.c new revision: 3.41; previous revision: 3.40 done Checking in nsprpub/pr/src/io/prsocket.c; /cvsroot/mozilla/nsprpub/pr/src/io/prsocket.c,v <-- prsocket.c new revision: 3.62; previous revision: 3.61 done Checking in nsprpub/pr/src/md/os2/Makefile.in; /cvsroot/mozilla/nsprpub/pr/src/md/os2/Makefile.in,v <-- Makefile.in new revision: 1.17; previous revision: 1.16 done Checking in nsprpub/pr/src/md/os2/objs.mk; /cvsroot/mozilla/nsprpub/pr/src/md/os2/objs.mk,v <-- objs.mk new revision: 1.8; previous revision: 1.7 done Checking in nsprpub/pr/src/md/os2/os2_errors.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2_errors.c,v <-- os2_errors.c new revision: 3.10; previous revision: 3.9 done Checking in nsprpub/pr/src/md/os2/os2cv.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2cv.c,v <-- os2cv.c new revision: 3.16; previous revision: 3.15 done Checking in nsprpub/pr/src/md/os2/os2io.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2io.c,v <-- os2io.c new revision: 3.19; previous revision: 3.18 done Checking in nsprpub/pr/src/md/os2/os2poll.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2poll.c,v <-- os2poll.c new revision: 3.15; previous revision: 3.14 done Checking in nsprpub/pr/src/md/os2/os2sock.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2sock.c,v <-- os2sock.c new revision: 3.18; previous revision: 3.17 done Checking in nsprpub/pr/src/md/os2/os2thred.c; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2thred.c,v <-- os2thred.c new revision: 3.20; previous revision: 3.19 done Removing nsprpub/pr/src/md/os2/os2vacpp.asm; /cvsroot/mozilla/nsprpub/pr/src/md/os2/os2vacpp.asm,v <-- os2vacpp.asm new revision: delete; previous revision: 1.7 done Checking in nsprpub/pr/src/misc/prnetdb.c; /cvsroot/mozilla/nsprpub/pr/src/misc/prnetdb.c,v <-- prnetdb.c new revision: 3.57; previous revision: 3.56 done Checking in nsprpub/pr/src/misc/prtime.c; /cvsroot/mozilla/nsprpub/pr/src/misc/prtime.c,v <-- prtime.c new revision: 3.32; previous revision: 3.31 done Checking in nsprpub/pr/tests/Makefile.in; /cvsroot/mozilla/nsprpub/pr/tests/Makefile.in,v <-- Makefile.in new revision: 1.52; previous revision: 1.51 done Checking in nsprpub/pr/tests/attach.c; /cvsroot/mozilla/nsprpub/pr/tests/attach.c,v <-- attach.c new revision: 3.13; previous revision: 3.12 done Checking in nsprpub/pr/tests/prpoll.c; /cvsroot/mozilla/nsprpub/pr/tests/prpoll.c,v <-- prpoll.c new revision: 3.12; previous revision: 3.11 done Checking in nsprpub/pr/tests/sel_spd.c; /cvsroot/mozilla/nsprpub/pr/tests/sel_spd.c,v <-- sel_spd.c new revision: 3.14; previous revision: 3.13 done Checking in nsprpub/pr/tests/sigpipe.c; /cvsroot/mozilla/nsprpub/pr/tests/sigpipe.c,v <-- sigpipe.c new revision: 3.9; previous revision: 3.8 done Checking in nsprpub/pr/tests/sleep.c; /cvsroot/mozilla/nsprpub/pr/tests/sleep.c,v <-- sleep.c new revision: 3.8; previous revision: 3.7 done Checking in nsprpub/pr/tests/testfile.c; /cvsroot/mozilla/nsprpub/pr/tests/testfile.c,v <-- testfile.c new revision: 3.17; previous revision: 3.16 done Checking in nsprpub/pr/tests/thrpool_server.c; /cvsroot/mozilla/nsprpub/pr/tests/thrpool_server.c,v <-- thrpool_server.c new revision: 3.8; previous revision: 3.7 done Checking in nsprpub/pr/tests/tmocon.c; /cvsroot/mozilla/nsprpub/pr/tests/tmocon.c,v <-- tmocon.c new revision: 3.12; previous revision: 3.11 done Checking in nsprpub/tools/Makefile.in; /cvsroot/mozilla/nsprpub/tools/Makefile.in,v <-- Makefile.in new revision: 1.13; previous revision: 1.12 done
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Hardware: Other → PC
Resolution: --- → FIXED
Target Milestone: 4.7 → 4.7.2
Comment on attachment 318221 [details] [diff] [review] Remove VACPP support and RAMSEM (with corrected os2vacpp.asm diff) r=wtc. Peter, I reviewed what you checked in on the NSPR trunk. It looks good. I checked in some minor white space adjustments. I am going to attach a patch next, which are some other changes I'd like to make but am less sure about.
Attachment #318221 - Flags: review+
1. Please confirm that HMTX is also recursive on OS/2. 2. In prlog.c, you removed the description of IBM's contribution. You should also remove IBM's copyright notice. I thought IBM's comment describing the purpose of the int3 is useful, so in this patch I propose that we add it back. But this requires restoring the description of IBM's contribution, too. If the int3 doesn't need a comment, we can just remove the IBM copyright notice. 3. In _pcos.h (not shown in this patch), your patch comments out the getopt declaration for OS/2. Is this because our declaration conflicts with the declaration of getopt in an OS/2 system header?
Wan-Teh, thanks for further cleaning up my cleanup. (In reply to comment #19) > Created an attachment (id=319065) [details] > 1. Please confirm that HMTX is also recursive on OS/2. I didn't actually know what "recursive" meant regarding a mutex. Google tells me that this means that mutexes can be requested more than once by the same thread. The docs don't say what happens when that is done on OS/2, so perhaps that comment should be removed entirely. > 2. In prlog.c, you removed the description of IBM's contribution. > You should also remove IBM's copyright notice. Weird licensing stuff. ;-) I just removed that line because it contained "DebugBreak()" which was removed as a function. > I thought IBM's comment describing the purpose of the int3 > is useful, so in this patch I propose that we add it back. But > this requires restoring the description of IBM's contribution, > too. > > If the int3 doesn't need a comment, we can just remove the > IBM copyright notice. The comment about int3 could be helpful, yes. But strictly, IBM didn't add "int $3" in 2000, that came later, with revision 3.29 of the file on 2003-09-15. Don't really know what the best approach is, especially since I don't understand IBM's legal requirements about contributions. Seems that this practise was stopped later. Perhaps add the original comment back in as it was and add a line that IBM added int3 for GCC in 2003? > 3. In _pcos.h (not shown in this patch), your patch comments out the > getopt declaration for OS/2. Is this because our declaration conflicts > with the declaration of getopt in an OS/2 system header? In the headers of our GCC, yes. This isn't a change, though, as we have used the XP_OS2_EMX route for the last 3-4 years.
Removed the IBM copyright notice. Moved IBM to the Contributors section in the tri-license header. Don't add the int3 comment back so that the only line of code coming from IBM is: asm("int $3"); which is a standard inline assembly statement.
Comment on attachment 319073 [details] [diff] [review] Minor tweaks v2 Yes, that should do it, thanks!
Attachment #319073 - Flags: review?(mozilla) → review+
Comment on attachment 319073 [details] [diff] [review] Minor tweaks v2 I checked in this patch. Checking in include/md/_os2.h; /cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v <-- _os2.h new revision: 3.38; previous revision: 3.37 done Checking in src/io/prlog.c; /cvsroot/mozilla/nsprpub/pr/src/io/prlog.c,v <-- prlog.c new revision: 3.42; previous revision: 3.41 done
Wan-Teh, Re: comment 19, HMTX are "recursive by default" according to http://www.ibm.com/developerworks/linux/library/l-osmig1.html . I didn't see any way to make them non-recursive. DosCreateMutexSem doesn't have a flag for this purpose.
Thanks, Julien. I added that comment back. Checking in _os2.h; /cvsroot/mozilla/nsprpub/pr/include/md/_os2.h,v <-- _os2.h new revision: 3.39; previous revision: 3.38 done
You need to log in before you can comment on or make changes to this bug.