458112 - [OS/2] get rid of VisualAge in LDAP C SDK

Status: RESOLVED FIXED

(Directory :: LDAP C SDK, defect)

Description

[Walter Meinl]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.1b1pre) Gecko/20080930011334 Minefield/3.1b1pre
Build Identifier: 

removal of Vacpp OS/2 from the LDAP C SDK.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Walter Meinl]

Attachment: the patch

a bit more removed in config/os2.mk

Comment 2

[Peter Weilbacher]

Comment on attachment 341335 [details] [diff] [review]
the patch

>-#
>-# On OS/2 we proudly support gbash...
>-#
>-SHELL = GBASH.EXE
>+XP_OS2   = 1

Did you remove the SHELL setting here (config/OS2.mk) on purpose?

Can you further remove MOZ_OS2_EMX_OBJECTFORMAT? I don't even see if it is still getting set somewhere...

Is MOZ_EMXTAG still needed?

> #ifdef XP_OS2
> #include <sys/select.h>
>-#endif
>-#ifdef XP_OS2_EMX
> /*
>  * EMX-specific tweaks:
>  *    o Use stricmp instead of strcmpi.
>  */
> #define strcmpi stricmp
> #endif

Can we not do without that EMX tweak?

Looks good to me otherwise.

Comment 3

[Dave Yeo]

Comment on attachment 341335 [details] [diff] [review]
the patch


+    OS_LIBS="-lsocket"

We don't need -lsocket anymore.

-SHELL = GBASH.EXE
+XP_OS2   = 1
 
 CC			= icc -q -DXP_OS2 -N10
 CCC			= icc -q -DXP_OS2 -DOS2=4 -N10
 LINK			= -ilink
 AR			= -ilib /noignorecase /nologo /Out:$(subst /,\\,$@)
 RANLIB 			= @echo RANLIB
 BSDECHO 		= @echo BSDECHO
 NSINSTALL 		= nsinstall
@@ -114,66 +95,48 @@ DEFINES += -D_PR_GLOBAL_THREADS_ONLY -DB
 ifdef MOZ_LITE
 OBJDIR_NAME 		= $(subst OS2,NAV,$(OS_CONFIG))_$(MOZ_OS2_TOOLS)$(MOZ_EMXTAG)$(OBJDIR_TAG).OBJ
 else
 OBJDIR_NAME 		= $(OS_CONFIG)_$(MOZ_OS2_TOOLS)$(MOZ_EMXTAG)$(OBJDIR_TAG).OBJ
 endif
 
 OS_DLLFLAGS 		= -nologo -DLL -FREE -NOE
 
-ifdef XP_OS2_VACPP

Should not the above visualage definitions be removed? I also agree that the shell setting should be removed.
All the errors in ldappr-error.c are now unneeded, they're all declared in sys/errno.h

Comment 4

[Walter Meinl]

Digging deep in mxr of the 1.8 Branch I found the following comment for the c-directory/config directory
This part of of the tree taken from NSPR 4.1.  The
NSPR config tree was tagged with:
cvs tag ldapcsdk_branch50-config
The version of NSPR this was taken from:
NSPRPUB_RELEASE_4_1 (revision: 3.23)
I'm pretty sure that all the {platform}.mk files were intended for building a standalone version. However, it looks like they were never overhauled to be able to really can build a standalone version. Some of the platform.mk files are still unchanged, some were changed occasionally when configure.in was also changed. 
Anyway, if I'm right this explains why removing the shell variable from OS2.mk doesn't affect the build, since it uses only configure and probably rules.mk and Makefile.in. I am tempted to leave OS2.mk as it is with the future option to change it, when a standalone version is desired.
Would be nice if a csdk maintainer can speak up

Comment 5

[Walter Meinl]

Attachment: Update of the patch including configure

I downloaded the standalone source tarball of the sdk. In order to use the Makefile.client and {platform}.mk files this tarball contains an additional Makefile in the toplevel directory that is _not_ present in the mozilla tree as such. Moreover, according to the LDAP C SDK wiki the method using the {platform}.mk files is deprecated and the autoconf way should be used. Nevertheless I tried the deprecated method and found out that OS2.mk would need much more overhaul than only to remove the vacpp lines and what else was suggested in comment #2 and comment #3. That's certainly another bug, and probably not worth to investigate deeper. This {platform}.mk Makefile.client way of building was originally carried over from nspr, where it was removed later in favor of the autoconf way, therefore, it may be removed from the LDAP C SDK sources as well sometimes.
The attached patch therefore omits any patches for config/OS2.mk and config/Makefile.client. It works however for building the LDAP C SDK standalone using the autoconf method and most important also during building Seamonkey and Thunderbird.
Finally, the updated patch contains a recreated configure file as I saw that it is desired from the maintainers (btw, it was not updated after the last changes for MINGW) and the last patch missed as Dave pointed out the removal of vacpp in build.mk and config/rules.mk

Comment 6

[Peter Weilbacher]

Comment on attachment 343691 [details] [diff] [review]
Update of the patch including configure

I think you should still include the changes to OS2.mk and Makefile.client, even though it won't build with that method. Just so that searching for VACPP and EMX in the sources doesn't turn up this stuff any more. I agree that at this point we shouldn't try to get it working but doing these changes certainly also doesn't break it any more than it is now.

Sorry for taking so long...

Comment 7

[Walter Meinl]

Attachment: Update of the patch including os2.mk + makefile.client

basically the same patch including the files mentioned. I removed a bit more in OS2.mk, those lines that were obviously related to the vacpp build system. It still does not build completely that way but at least a bit more than before, somehow it takes ld for linking and than fails.

Comment 8

[Walter Meinl]

Attachment: Update of the patch including os2.mk + makefile.client

corrected the patch targets to apply w/o problems

Comment 9

[Peter Weilbacher]

Comment on attachment 345790 [details] [diff] [review]
Update of the patch including os2.mk + makefile.client

Looks good from the OS/2 POV. (Perhaps you can get linking working with Makefile.client when adding -Zomf to XLDOPTS as in Makefile.in?)

I guess sr by Mark will tell us if the generated configure should be included.

Comment 10

[Mark Smith [:mcs]]

Comment on attachment 345790 [details] [diff] [review]
Update of the patch including os2.mk + makefile.client

The changes look OK, but I am not a superreviewer (we generally do not require sr for LDAP C SDK changes until they will affect TBird or another application).

I do not think the non-autoconf build process is really supported any longer.

Also, Rich Megginson should review and comment on the configure changes.

Comment 11

[Walter Meinl]

Comment on attachment 345790 [details] [diff] [review]
Update of the patch including os2.mk + makefile.client

Richm, any objections to the code changes, particularly the regenerated configure?

Comment 12

[Rich Megginson]

Looks fine to me.

Comment 13

[Walter Meinl]

(In reply to comment #12)
> Looks fine to me.
Peter, for me this looks like r+. I checked the CVS History, it appears that you have the rights to check into the directory/c-sdk repository.

Comment 14

[Peter Weilbacher]

Comment on attachment 345790 [details] [diff] [review]
Update of the patch including os2.mk + makefile.client

richm said this was OK.

Comment 15

[Peter Weilbacher]

Checked in to LDAP trunk.

It seems that the 6.0.x releases that are used in comm-central are taken from trunk, so I guess we will get this with the next update (I don't think we need to create a new release just for this), and this is fixed.