Closed Bug 444495 Opened 17 years ago Closed 16 years ago

getenv / putenv compiled out in WinCE NSPRPUB

Categories

(Firefox for Android Graveyard :: General, defect, P2)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
x86
Windows CE
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED INVALID
Milestone:
fennec1.0b1

People

(Reporter: wolfe, Assigned: wolfe)

References

Details

(Keywords: mobile)

Attachments

(1 file, 7 obsolete files)

Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers
(deleted), patch
dougt
: review+
Details | Diff | Splinter Review
Within pr/src/md/windows/ntmisc.c, function calls to getenv() and putenv() result in no actual code -- so environment setting and getting fails. This bug requires patches for BOTH mozilla-central AND NSPRPUB - hence the two patch files attached. I make a newly-named functions that do the work of getenv() and putenv(), and call those functions within pr/src/md/windows/ntmisc.c
THIS IS A PATCH TO NSPRPUB -- NOT TO MOZILLA-CENTRAL (at least, until NSPRPUB is incorporated into mercurial repositories).
Assignee: nobody → wtc
Component: General → NSPR
Product: Fennec → NSPR
QA Contact: general → nspr
Version: 1.0 → other
Version: other → 4.7.1
Keywords: mobile
John, the mozce shunt use to (and should) provide getenv() and putenv(). I do not see any reason for either of these patches. What are you attempting to do?
Doug, you are absolutely correct - the shunt does provide implementations of the getenv() and putenv() functions. Unfortunately, the WinCE headers provide their own in-line stubs for these functions - rendering the getenv() / putenv() function calls in nsprpub/pr/src/md/windows/ntmisc.c into stubs that do nothing. Since changing the headers provided by Microsoft is a big no-no, the simplest solution was to provide a second set of entry points into the shunt's getenv() / putenv() functions.
do this: in the shunt, redefine getenv and putenv as: #ifdef getenv #undef getenv #define getenv mozce_getenv #endif We do this for a few other APIs. (eg. http://mxr.mozilla.org/mozilla-central/source/build/wince/shunt/include/mozce_shunt.h#87)
Comment on attachment 328803 [details] [diff] [review] Patch for mozilla-central code for new WinCE getenv() / putenv() functions per doug's comments
Attachment #328803 - Flags: review-
Comment on attachment 328804 [details] [diff] [review] WinCE NSPRPUB getenv() / putenv() newly-named function usage per doug's comments
Attachment #328804 - Flags: review-
As per doug suggestion, with more lines of code around the changes - easier to see what is going on.
Attachment #328803 - Attachment is obsolete: true
NSPRPUB DIFF, modified as per dougt, done in Unified instead of Context formatting
Attachment #328804 - Attachment is obsolete: true
Comment on attachment 329601 [details] [diff] [review] Patch for mozilla-central code for new WinCE getenv() / putenv() functions Same DIFF, just done in Unified instead of Context formatting
Attachment #329601 - Attachment description: Same DIFF, just done in Unified instead of Context formatting → Patch for mozilla-central code for new WinCE getenv() / putenv() functions
Comment on attachment 329601 [details] [diff] [review] Patch for mozilla-central code for new WinCE getenv() / putenv() functions ><HTML><HEAD><script xmlns="http://www.w3.org/1999/xhtml" src="chrome://skype_ff_toolbar_win/content/injection_graph_func.js" id="injection_graph_func" charset="utf-8"/></HEAD><BODY><PRE>diff -r c42a579a03c5 pr/src/md/windows/ntmisc.c >--- a/pr/src/md/windows/ntmisc.c Tue May 20 15:13:59 2008 -0700 >+++ b/pr/src/md/windows/ntmisc.c Mon Jul 14 17:15:57 2008 -0400 >@@ -34,20 +34,25 @@ > * the terms of any one of the MPL, the GPL or the LGPL. > * > * ***** END LICENSE BLOCK ***** */ > > /* > * ntmisc.c > * > */ > > #include "primpl.h" >+ >+#ifdef WINCE >+#define getenv(name) _MD_GetEnv(name) >+#define putenv(name) _MD_PutEnv(name) >+#endif > > char *_PR_MD_GET_ENV(const char *name) > { > return getenv(name); > } > > /* > ** _PR_MD_PUT_ENV() -- add or change environment variable > ** > **
Darn it - I grabbed the wrong file from my hard disk for the new Unified DIFF patch (attachment 329602 [details] [diff] [review]). Sorry for the confusion. This patch just does an #IFDEF WINCE / #DEFINE getenv(x) _MD_getenv(x) / ... at the top of the source file instead of modifying the getenv() and putenv() function calls within pr/src/md/windows/ntmisc.c source file.
Attachment #329602 - Attachment is obsolete: true
john, don't mess with nspr code. _MD_GetEnv is really a nspr internal that doesn't need to be modified. I am pretty sure that if we just implement getenv and putenv, the nspr code will use the right stuff
Doug, I created _MD_GetEnv() and _MD_PutEnv() because I was trying to follow what looked like a "standard" naming convention. I guess I was successful in making that chunk of code "blend" in with all the other NSPR internal functions. All I really needed was two functions whose names were NOT getenv and putenv, so I could call those functions without the compiler in-lining the stubs contained within the Windows Mobile SDK include files. Then I use the not-named-getenv-and-putenv function names in my newly-created NSPR patch, because the compilation of nsprpub/pr/src/md/windows/ntmisc.c is where the Microsoft SDK include's in-line stubbing of the functions getenv() and putenv() actually happen. Does this make sense? If so, how should I modify my patches to make them more in-line with what people expect from the WinCE stub and NSPRPUB?
Comment on attachment 329603 [details] [diff] [review] Patch for NSPRPUB code using shunt _MD_getenv() / _MD_putenv() functions for WinCE >+#ifdef WINCE >+#define getenv(name) _MD_GetEnv(name) >+#define putenv(name) _MD_PutEnv(name) >+#endif You should name your implementations mozce_getenv and mozce_putenv, and define getenv(name) as mozce_getenv(name) and putenv(name) as mozce_putenv(name).
Changed _MD_GetEnv to mozce_GetEnv / _MD_PutEnv to mozce_PutEnv Added defines to mozce_shunt.h, which is included by compiler shims - this means no more modification to NSPRPUB. Obsoleted both previous DIFF UNIFIED patches. Thanks to everyone for suggestions on how to modify my patches.
Attachment #329601 - Attachment is obsolete: true
Attachment #329603 - Attachment is obsolete: true
Attachment #330587 - Attachment description: Fix for getenv/putenv in-line defined out by WinMobile6 SDK headers → Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers
I had accidentally left in the patch an extra modification inside NSPRPUB, which was rendered unnecessary by suggestions of Doug T. Although review should trounce on the delta to nsprpub/pr/src/md/windows/ntmisc.c, now they do not have to.
Attachment #330587 - Attachment is obsolete: true
Attachment #330593 - Flags: review+
Comment on attachment 330593 [details] [diff] [review] Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers (Darn unsaved buffer) Today is not my day! I meant to mark this patch as up-for-review, not reviewed. Thank you for the catch, Nelson.
Attachment #330593 - Flags: review+ → review?
mozce_getenv and mozce_putenv are more consistent with the convention used in mozce_shunt.h: mozce_foo for the system function 'foo', with the exact same capitalization.
I had convinced myself that the convention within mozce_shunt.h was more along the lines of capitalizing the first letter of each word within a function name. Thank you, Wan-Teh Chang, for pointing out my mistake. I am uploading a revised patch now.
Modified new mozce function names as follows: mozce_GetEnv => mozce_getenv mozce_PutEnv => mozce_putenv This matches the convention throughout the rest of mozce_shunt.h to replace function "foo" with an internal function mozce_foo
Attachment #330593 - Attachment is obsolete: true
Attachment #330593 - Flags: review?
Attachment #330641 - Flags: review?(wolfe)
Comment on attachment 330641 [details] [diff] [review] Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers I'll bet this review request was intended to go to someone else.
Attachment #330641 - Flags: review?(wolfe) → review?(doug.turner)
over to the right component, and the very least. ;-)
Assignee: wtc → nobody
Component: NSPR → General
Product: NSPR → Fennec
QA Contact: nspr → general
Version: 4.7.1 → 1.0
Comment on attachment 330641 [details] [diff] [review] Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers " " is alot of spaces. nit: maybe just one like the rest of the file. Will this cause link errors: MOZCE_SHUNT_API char* getenv(const char* inName) +MOZCE_SHUNT_API int putenv(const char *a) Otherwise, looks fine. r= assuming the above doesnt cause link errors.
Attachment #330641 - Flags: review?(doug.turner) → review+
Are getenv and putenv really defined as macros by some WinCE system header? If so, could you paste the definitions of the macros here? Thanks.
Assignee: nobody → wolfe
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec M7
Blocks: 432792
what is left to do here?
wolfe, can you past the defines here as wtc asked? Thanks!
Target Milestone: Fennec M7 → Fennec A1
Target Milestone: Fennec A1 → Fennec A2
Target Milestone: Fennec A2 → Fennec A3
The revised implementation of the WinCE Shunt Library removed the need for this bug fix - since it slightly re-implemented the getenv() and putenv() functions. Therefore, this bug should be marked as resolved.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Invalid rather than fixed in this case, I think.
Resolution: FIXED → INVALID
verified in alpha2 bits. This fix is used for unittests.
Status: RESOLVED → VERIFIED
sorry for bug spam. Many of the bugs which are marked invalid, I see comments telling it occurred in one version or other. But later it was fixed due to 1) by backingout the patch which made regression or 2) by fixing some other bug. So if we can identify the bug/patch/reason then we should state that and mark those as FIXED. If not mark as WORKSFORME in that case.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: