getenv / putenv compiled out in WinCE NSPRPUB

VERIFIED INVALID

Status

Firefox for Android Graveyard
General
P2
normal
VERIFIED INVALID
10 years ago
9 years ago

People

(Reporter: wolfe, Assigned: wolfe)

Tracking

({mobile})

Trunk
fennec1.0b1
x86
Windows CE
mobile
Bug Flags:
blocking-xul-fennec1.0 +

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Created attachment 328803 [details] [diff] [review]
Patch for mozilla-central code for new WinCE getenv() / putenv() functions

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
(Assignee)

Comment 1

10 years ago
Created attachment 328804 [details] [diff] [review]
WinCE NSPRPUB getenv() / putenv() newly-named function usage

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

Updated

10 years ago
Keywords: mobile

Comment 2

10 years ago
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?
(Assignee)

Comment 3

10 years ago
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.

Comment 4

10 years ago
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 5

10 years ago
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 6

10 years ago
Comment on attachment 328804 [details] [diff] [review]
WinCE NSPRPUB getenv() / putenv() newly-named function usage

per doug's comments
Attachment #328804 - Flags: review-
(Assignee)

Comment 7

10 years ago
Created attachment 329601 [details] [diff] [review]
Patch for mozilla-central code for new WinCE getenv() / putenv() functions

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
(Assignee)

Comment 8

10 years ago
Created attachment 329602 [details] [diff] [review]
WinCE NSPRPUB getenv() / putenv() newly-named function usage

NSPRPUB DIFF, modified as per dougt, done in Unified instead of Context formatting
(Assignee)

Updated

10 years ago
Attachment #328804 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
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
(Assignee)

Comment 10

10 years ago
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
> **
> **
(Assignee)

Comment 11

10 years ago
Created attachment 329603 [details] [diff] [review]
Patch for NSPRPUB code using shunt _MD_getenv() / _MD_putenv() functions for WinCE

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

Comment 12

10 years ago
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
(Assignee)

Comment 13

10 years ago
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 14

10 years ago
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).
(Assignee)

Comment 15

10 years ago
Created attachment 330587 [details] [diff] [review]
Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers

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
(Assignee)

Updated

10 years ago
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
(Assignee)

Comment 16

10 years ago
Created attachment 330593 [details] [diff] [review]
Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers (Darn unsaved buffer)

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+
(Assignee)

Comment 17

10 years ago
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?

Comment 18

10 years ago
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.
(Assignee)

Comment 19

10 years ago
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.
(Assignee)

Comment 20

10 years ago
Created attachment 330641 [details] [diff] [review]
Fix for getenv/putenv defined to nothing by WinMobile6 SDK headers

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?
(Assignee)

Updated

10 years ago
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)

Comment 22

10 years ago
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 23

10 years ago
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+

Comment 24

10 years ago
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.

Updated

10 years ago
Assignee: nobody → wolfe
Flags: blocking-fennec1.0+
Priority: -- → P2
Target Milestone: --- → Fennec M7

Updated

10 years ago
Blocks: 432792

Comment 25

10 years ago
what is left to do here?

Comment 26

10 years ago
wolfe, can you past the defines here as wtc asked?  Thanks!

Updated

10 years ago
Target Milestone: Fennec M7 → Fennec A1

Updated

10 years ago
Target Milestone: Fennec A1 → Fennec A2

Updated

10 years ago
Target Milestone: Fennec A2 → Fennec A3
(Assignee)

Comment 27

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 28

10 years ago
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

Comment 30

9 years ago
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.