483653 - unable to build certutil.exe for fennec/wince

Status: VERIFIED FIXED

(NSS :: Build, defect, P2)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

In getting the --enable-tests up and running for wince, I have ran into a list of hurdles that cause the build to fail.

First off, we don't have a stdin on wince.  So conio.h is not found and won't work.  

We can fix this by modifying keystuff.c (http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/certutil/keystuff.c):
1) for wince, remove the #include <conio.h>
2) for wince, remove the UpdateRNG(void) function
3) on line 559 remove the call to UpdateRNG().  



Second, once you remove the stdin dependencies, we need to fix the build system.  There are two pieces we need to fix:
1) platlibs.mk (http://mxr.mozilla.org/mozilla-central/source/security/nss/cmd/platlibs.mk#251)
 for wince, we need to remove the -l<libname>, and make it just libname.lib.  Also, we need to change the -L<path> to -LIBPATH:<path>
EXTRA_SHARED_LIBS += \
	-LIBPATH:$(DIST)/lib \
	ssl3.lib \
	smime3.lib \
	nss3.lib \
	nssutil3.lib \
	-LIBPATH:$(NSPR_LIB_DIR) \
	plc4.lib \
	plds4.lib \
	nspr4.lib \
	$(NULL)


2) ruleset.mk (http://mxr.mozilla.org/mozilla-central/source/security/coreconf/ruleset.mk#140)
 for wince, we need to change it something like this:
#    PROGRAMS := $(addprefix $(OBJDIR)/, $(PROGRAMS:%=%$(JDK_DEBUG_SUFFIX)$(PROG_SUFFIX)))
    PROGRAM := $(addprefix 'cd  $(OBJDIR); pwd -w' /, $(PROGRAM)$(JDK_DEBUG_SUFFIX)$(PROG_SUFFIX))

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

I think the solution is simply not to build mozilla/security/nss/cmd 
or anything under it on WinCE.
I believe you could not use any of those command line utilities even if
you did build them, so why build them?

Comment 2

[Joel Maher ( :jmaher ) (UTC -8)]

We already have this to remove the general building of the cmd directory:
http://mxr.mozilla.org/mozilla-central/source/security/nss/Makefile#61

If we go that route, we need to edit:
http://mxr.mozilla.org/mozilla-central/source/security/manager/Makefile.in

to remove these instances:
$(MAKE) -C $(topsrcdir)/security/nss/cmd/...

by using something like:
ifneq ($(OS_TARGET),WINCE)
  $(MAKE) -C $(topsrcdir)/security/nss/cmd/...
endif

Comment 3

[(not currently active) Ted Mielczarek]

Note that as is, you won't be able to run all the mochitests without these utils.

Comment 4

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proposed patch

This patch doing the following:

* Implement wmmain and define main as NS_internal_main, this is the same technique in toolkit/xre/nsWindowsWMain.cpp 
* Add -ENTRY:mainWCRTStartup to LDFLAGS
* In platlibs.mk set librarie paths same as WINNT.
* Use getc instead of getch in keystuff.c.

Comment 5

[Hiroyuki Ikezoe (:hiro)]

The patch depends on patches in Bug 491795 and 491796. 
If both patches are applied, attachment 376166 [details] [diff] [review] makes certutil.exe correctly and seems to work fine. 

I confirmed it on PPC command shell.

http://www.microsoft.com/downloads/details.aspx?FamilyID=74473fd6-1dcc-47aa-ab28-6a2b006edfe9&DisplayLang=en

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

using this full set of patches, I need to build with /entry:wmain and it works.  Can we change the #ifdef WINCE to have a #else clause and keep it at main()?  

#ifdef WINCE
...
int main()
...
#else
int main()
#endif

I don't like #ifdef's in code in general.  So either we add a /entry:wmain (which would require a #ifdef WINCE in the makefile) or adjust this patch.

Comment 7

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #6)
> I don't like #ifdef's in code in general.  So either we add a /entry:wmain
> (which would require a #ifdef WINCE in the makefile) or adjust this patch.

Which Makefile do you mean?

Comment 8

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #6)

>  Can we change the #ifdef WINCE to have a #else clause and keep it at main()?  
> 
> #ifdef WINCE
> ...
> int main()
> ...
> #else
> int main()
> #endif

If you want to do this, main function will not be needed for WINCE and certutil_main will be invoked directory in wmain and no need to define main macro.

#ifdef WINCE
#include <windows.h>
int
wmain (int argc, WCHAR **wargv)
{
...
  ret = certutil_main(argc, argv, TRUE);
...
  PR_Cleanup();
  return ret;
}
#else
int
main(int argc, char **argv)
{
..
}
#endif

Do you prefer this?

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

I am open to the method you did originally except I had trouble building with it.  For some reason the '#define main NS_internal_main' didn't seem to work for my build environment (https://wiki.mozilla.org/Mobile/Build/Windows_Mobile_Build_Instructions).

Maybe the solution is fixing the makefile to set 'WIN32_EXE_LDFLAGS += -ENTRY:mainWCRTStartup'.  I tried to figure out which makefile.in to edit and didn't have much luck.

Comment 10

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

Append -ENTRY option out of BUILD_OPT ifdef block.

I hope the patch fixes your issue. I do not confirm yet since I am facing another issue now.

Comment 11

[Hiroyuki Ikezoe (:hiro)]

Attachment: Bit cleaner patch

Add wincemain.c including wmain function and source file which has main function includes wincemain.c.

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Attachment: The diff for mozilla-central

This is for Joel.

Comment 13

[Nelson Bolyard (seldom reads bugmail)]

Guys, 
Recently, all these same issues (building test programs on WinCE) were solved
for NSPR.  See bug 469083.

I very much want to see this bug be solved in the same way in NSS as in NSPR.
The solution used for NSPR was virtually the diametric opposite of what you're
proposing to do here.  Please study the patches attached to bug 469083 and 
write a patch here for NSS that uses the same techniques.  Please study the 
comment history of that bug to understand the rationale for the methods used.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 380316 [details] [diff] [review]
Bit cleaner patch

This problem can be solved without needing this:

>+LDFLAGS     += -ENTRY:mainWCRTStartup

and without needing to add an ifdef'ed #include to every .c file
in cmd.  The NSPR bug I cited above shows how.

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

The patch above added ifdef'ed #includes to only two files.  That's fine if
you only intend to build those two commands, but not if you intend to build
all the cmds in nss/cmd.

Is it your intent to build a subset of the commands in nss/cmd for WinCE?
If so, do you have a patch to one of the makefiles in nss/cmd to change 
the set of subdirectories that is built?

Comment 16

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #14)
> (From update of attachment 380316 [details] [diff] [review])
> This problem can be solved without needing this:
> 
> >+LDFLAGS     += -ENTRY:mainWCRTStartup
> 
> and without needing to add an ifdef'ed #include to every .c file
> in cmd.  The NSPR bug I cited above shows how.

This is definitely wrong. The NSPR program does not handle its arguments. So the NSPR program looks working fine at a glance.
Without this entry option, program can not be handle arguments correctly on WinCE.

Please see bug 492307.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Attachment: Proof what I mentioned in comment #16

Current NSPR tests for WinCE can not be linked due to missing main symbol. (See 492307)

This patch avoids it and output number of program arguments.

If you run the wince_tester.exe, you will see an incredible big number. In my case it was 1571395190.

Comment 18

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch of attachment 380336

Joel pointed me out on IRC that wincemain.c should be in where is indicated include path.
So wincemain.c is now dist/private/nss. I am not sure it is right place but I can not find any other appropriate place.

Comment 19

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #15)
> The patch above added ifdef'ed #includes to only two files.  That's fine if
> you only intend to build those two commands, but not if you intend to build
> all the cmds in nss/cmd.

No I do not intend so.  I mistook the two programs are the all. It is something wrong. I have to investigate it.

Comment 20

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #19)
> (In reply to comment #15)
> > The patch above added ifdef'ed #includes to only two files.  That's fine if
> > you only intend to build those two commands, but not if you intend to build
> > all the cmds in nss/cmd.
> 
> No I do not intend so.  I mistook the two programs are the all. It is something
> wrong. I have to investigate it.

I noticed that security/manager/Makefile.in make these two programs. I have to build nss without fennec.

Comment 21

[(not currently active) Ted Mielczarek]

The Mozilla build process intentionally only builds certutil and pk12util, since those are the only tools we need. However, as Nelson said, if you're going to patch NSS, you should do it in a manner that fixes everything correctly.

Comment 22

[Hiroyuki Ikezoe (:hiro)]

Thank you for your advices. I am working on this now.

Comment 23

[Joel Maher ( :jmaher ) (UTC -8)]

The last patch I see fails during link on /entry:main.  I am not sure which approach we are taking to change it to /entry:wmain.

Comment 24

[Hiroyuki Ikezoe (:hiro)]

I am sorry, I missed a part of WINCE.mk and wincemain.c itself in the patch. 

Now I am going to take a different approach. I will attach a new patch later.

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Attachment: Another approach

This approach is a similar way what I proposed in bug 491994 comment #0.

This patch creates a new library (libwincemain.lib) which has wmain function. And each program links this library. So we does not need to include wmain.c any more in each program source.

Comment 26

[Hiroyuki Ikezoe (:hiro)]

I filed a new bug concerned to missing functions which are used in security/cmd on WinCE. See Bug 495652.

Comment 27

[Joel Maher ( :jmaher ) (UTC -8)]

using the latest attachment from comment #25, I was able to do a clean build (with --enable-tests) from scratch and get certutil and pk12util created.  This of course uses the other patches required to fix other issues.

Hiroyuki, thanks for continuing to post patches.  

If we agree on the approach taken in this latest patch, I would like to get a few of these patches checked in so we can start building with --enable-tests.

Comment 28

[Hiroyuki Ikezoe (:hiro)]

I filed a new bug (bug 495717) for fixing part of keystuff.c in order to get review step by step.

Comment 29

[Nelson Bolyard (seldom reads bugmail)]

Hiroyuki, please just keep one NSS bug for this issue.  
You need not file one big patch that fixes everything at once.
You can attach a series of patches to this bug, for NSS, and get them 
reviewed one at a time, if you wish.  
I'd rather have all the patches that fix one central issue all claim to 
be fixes for the same bug number.  Thanks.

Comment 30

[Hiroyuki Ikezoe (:hiro)]

I see. I am sorry for the inconvenience. I will attach patches here from now on.

Comment 31

[Hiroyuki Ikezoe (:hiro)]

Attachment: Add a library which has wmain function and link the library

This patch is a part of fix to handle command line arguments correctly.

This patch is slightly changed from attachment 38054 [details] [diff] [review].
 * remove the fix for keystuff.c
 * remove the fix for platlib.mk and nss/Makefile (This is another issue)
 * remove some garbage from libwincemain/Makefile (The garbage slipped into when copy-n-pate)
 * use CP_ACP instead of CP_UTF8 in wincemain.c. This is utterly my fault. CP_UTF8 will cause some problems.
 * use malloc and free instead of PORT_XX. This wmain function needs only for WinCE so PORT_XX functions are not needed here.

Comment 32

[Hiroyuki Ikezoe (:hiro)]

Attachment: Fix for mozilla-central/security/manager/Makefile.in

Comment 33

[Hiroyuki Ikezoe (:hiro)]

I have to explain why I do not take the similar way as test programs in NSPR.

* /FI option is not so useful for NSS commands.

 Each test program in NSPR consists of a C source file, so /FI option does not cause any side effect to include a file which has wmain function. But most of NSS commands consists of multiple C source file. So, if we use /FI option, we need a different tag from CSRCS to set /FI option only for a C source file which has main function.

* NSS commands need to handle command line arguments correcty.
 Test programs in NSPR does not need command line arguments, but NSS commands is 
useless unless command line arguments are not obtained correctly. So we need to implemenet wmain function to obtain command line arguments correctly.

Comment 34

[Hiroyuki Ikezoe (:hiro)]

Change blocking bug number because bug 495717 has been fixed.

Comment 35

[Joel Maher ( :jmaher ) (UTC -8)]

we should move this to be dependent upon bug 483654.  Since this bug (and the two blocking it) prevent --enable-tests to build, we should leave it there.  The bug related to wmain on windows mobile makes sense to be blocking 491903 since that is unrelated to getting the tests to build.

let me know if there is a reason we should leave it here.

Comment 36

[Hiroyuki Ikezoe (:hiro)]

Buildubg failure of certutil.exe has been already fixed. See bug 495717.
This bug is only concerned to wmain issue now. IMHO, the title of this bug should be renamed.

Comment 37

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #36)
> Buildubg failure of certutil.exe has been already fixed. See bug 495717.

Buildubg? I meant "Building". I must be tired..

Comment 38

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 380773 [details] [diff] [review]
Add a library which has wmain function and link the library

>Index: coreconf/WINCE.mk

1. Let's call these EXTRA_EXE_LD_FLAGS instead

>+WINCE_EXE_FLAGS += \
>+	$(DIST)/lib/$(LIB_PREFIX)libwincemain.$(LIB_SUFFIX) \
>+	-ENTRY:mainWCRTStartup

>Index: coreconf/rules.mk

2. Don't ifeq this.  Just add $(EXTRA_EXE_LD_FLAGS) unconditionally
to the command line.

> ifeq (,$(filter-out _WIN%,$(NS_USE_GCC)_$(OS_TARGET)))
>+ifeq (WINCE,$(OS_ARCH))
>+	$(MKPROG) $(subst /,\\,$(OBJS)) -Fe$@ -link $(LDFLAGS) $(subst /,\\,$(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS) $(WINCE_EXE_FLAGS))
>+else
> 	$(MKPROG) $(subst /,\\,$(OBJS)) -Fe$@ -link $(LDFLAGS) $(subst /,\\,$(EXTRA_LIBS) $(EXTRA_SHARED_LIBS) $(OS_LIBS))
>+endif


>Index: nss/cmd/Makefile

3. We've already got a library directory for commands.
Instead of adding another, just put this new stuff in 
nss/cmd/lib.  Put the lines below in nss/cmd/lib/config.mk

>+ifeq (WINCE,$(OS_ARCH))
>+DIRS += libwincemain
>+endif

>Index: nss/cmd/libwincemain/Makefile

4. This won't be necessary in nss/cmd/lib.
It already has a Makefile.

>Index: nss/cmd/libwincemain/config.mk

5. nss/cmd/lib already has one of these, but
you might want to modify it because you can't
put ifeq in manifest.mn because the symbols get
defined AFTER it is included.

>Index: nss/cmd/libwincemain/manifest.mn

6. nss/cmd/lib already has one.

>Index: nss/cmd/libwincemain/wincemain.c

7. put this in nss/cmd/lib, please.

Comment 39

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 380774 [details] [diff] [review]
Fix for mozilla-central/security/manager/Makefile.in

If you put the winmain functino in nss/cmd/lib, will this patch still be necessary?

Comment 40

[Hiroyuki Ikezoe (:hiro)]

Attachment: Revised patch

This patch is addressed Nelson's comment and slightly changed.

The changes are:

* Remove wlen variable from wmain. It was not used.
* Add WINCE into ifeq target in cmd/platlibs.mk. If this change does not exist, there are plenty unresolved symbols while linking.

Comment 41

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #39)
> (From update of attachment 380774 [details] [diff] [review])
> If you put the winmain functino in nss/cmd/lib, will this patch still be
> necessary?

Yes, it is not needed any more. Thank you.

Comment 42

[Joel Maher ( :jmaher ) (UTC -8)]

I am unable to build with the single patch.  In addition I added the two patches that are blocking this bug and that didn't help.  

I will figure out what is the problem and rearrange the bugs accordingly.  The goal is to build with --enable-tests so we can utilize the majority of the test code.

Comment 43

[Joel Maher ( :jmaher ) (UTC -8)]

after many trial and errors, I was able to get a build to work of windows mobile fennec with --enable tests.  It currently required 4 patches:
 - this bug, but patch https://bug483653.bugzilla.mozilla.org/attachment.cgi?id=380534
 - bug 491795 r+ blassey
 - bug 491796 r- blassey (/entry issue)
 - bug 491972 r+ benjamin

Note that the latest patch attached to this bug does not allow for building with --enable-tests due to a missing wincemain.obj while building pk12util.  

I am moving this bug back to blocking bug 483645 as it and its blocking bugs are all required to build successfully.

Comment 44

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #43)
> after many trial and errors, I was able to get a build to work of windows
> mobile fennec with --enable tests.  It currently required 4 patches:
>  - this bug, but patch
> https://bug483653.bugzilla.mozilla.org/attachment.cgi?id=380534

Yes, diff part of keystuff.c still needs in mozilla-central. We need to update nss in mozilla-central.


> Note that the latest patch attached to this bug does not allow for building
> with --enable-tests due to a missing wincemain.obj while building pk12util.  

OK. I will investigate.

> I am moving this bug back to blocking bug 483645 as it and its blocking bugs

You maybe post wrong bug id.

Comment 45

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 383440 [details] [diff] [review]
Revised patch

I'm inclined to give this r+.  I'll test build it tomorrow and if it passes, I'll commit it.

Comment 46

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 383440 [details] [diff] [review]
Revised patch

r=nelson
Checking in nss/cmd/platlibs.mk;     new revision: 1.64; previous revision: 1.63
Checking in coreconf/WINCE.mk;       new revision: 1.6;  previous revision: 1.5
Checking in coreconf/rules.mk;       new revision: 1.80; previous revision: 1.79
Checking in nss/cmd/lib/config.mk;   new revision: 1.4;  previous revision: 1.3
Checking in nss/cmd/lib/wincemain.c; initial revision: 1.1

Is this bug resolved/fixed now?  Or is there more to be done?

Comment 47

[Hiroyuki Ikezoe (:hiro)]

(In reply to comment #46)

> Is this bug resolved/fixed now? 

Yes, I think so. The last thing that we need to do for building tests on Wince is updating NSS tree in mozilla-central. I filed bug bug 499063 for it but I am not sure when next NSS is released.

Comment 48

[Joel Maher ( :jmaher ) (UTC -8)]

verified with 1.9.2 build and winmo alpha3