113818 - CCK UI and backend need to be separated

Status: VERIFIED FIXED

(CCK Graveyard :: CCK-General, defect)

Description

[Shannon Diener]

This bug tracks patches and discussions about separating CCK's UI from the backend

Comment 1

[Shannon Diener]

Attachment: patch

Comment 2

[Shannon Diener]

The patch I just attached contains my latest diffs for this bug.  

What this patch does is create a backend executable (ibengine.exe) where the
backend previously consisted of only dlls.  In order to sort out the
dependencies I've rearranged the source code to create three major parts: UI
(wizardmachine.exe), backend (ibengine.exe), and globals.dll (shared dll between
UI and backend).  The way these now work together:

 - The UI begins and collects all user data. Once finished it creates a cck.che.
 - The backend (ibengine.exe) is then called to run install builder tasks,
creating the customized build
 - The UI returns, displaying location of new customized build and finishes

Changes that were made:
 - Functions GetModulePath and FillGlobalWidgetArray are duplicated in both UI
and backend
 - ib/comp.cpp and comp.h are now in the globals directory and will be included
in globals.dll
 - engine.cpp has been added to the ib directory
 - "Customization is in Progress" dialog has been removed ... it lived in ib.cpp
 - ib.cpp function EraseDirectory has been moved to globals.cpp to be included
in globals.dll
 - A system call has been added to driver/interpret.cpp to call ibengine.exe

 

Comment 3

[Shannon Diener]

Attachment: diffs to comp.cpp - which has now moved to the globals directory with comp.h

Comment 4

[Shannon Diener]

Attachment: new file engine.cpp

Comment 5

[Shruti Vasudevan]

Hi Shannon,
Before reviewing, I had some questions based on your comments.
> -Functions GetModulePath and FillGlobalWidgetArray are duplicated in both UI
> and backend
I personally don't support this code duplication. Is there a specific reason?

> -"Customization is in Progress" dialog has been removed ... it lived in ib.cpp
Why has this been removed?

Comment 6

[Shannon Diener]

>> -Functions GetModulePath and FillGlobalWidgetArray are duplicated in both UI
>> and backend
>I personally don't support this code duplication. Is there a specific reason?

GetModulePath could probably be put in globals.dll ... Its now needed for both 
ibengine.exe and wizardmachine.exe ... does that sound okay?

FillGlobalWidgetArray is also needed for both but I'm concerned about this line:

value=theInterpreter->replaceVars((char *) (LPCSTR) value,NULL);

That I don't need for the backend but need for the UI ... suggestions welcome :-)
 


>> -"Customization is in Progress" dialog has been removed ... it lived in ib.cpp
>Why has this been removed?

It lived in ib.cpp (which I am now including in the backend) I'm not sure if we
really want any dialog boxes popping up in this area anymore ... again,
suggestions welcome 

Thanks for asking about both of these ... I mentioned them because I knew they
would be concerns.

Comment 7

[Shruti Vasudevan]

> GetModulePath could probably be put in globals.dll ... Its now needed for both 
> ibengine.exe and wizardmachine.exe ... does that sound okay?
What I think is to call function GetModulePath (present in wizardmachine.cpp) 
from  ib.cpp.

> -"Customization is in Progress" dialog has been removed ...
> I'm not sure if we really want any dialog boxes popping up in this area 
> anymore
Lets see what Tao suggests.

Comment 8

[Shannon Diener]

>What I think is to call function GetModulePath (present in wizardmachine.cpp) 
>from  ib.cpp.

Unfortunately, this would mean that the backend depends on the UI again ...

Comment 9

[Shannon Diener]

Attachment: updated patch ... everything except comp.cpp and engine.cpp (which remain the same)

Comment 10

[Shannon Diener]

I've just attached an updated patch with the following changes from the last patch:
 - GetModulePath duplication has been removed ... it has been removed from
wizardmachine.cpp and ib.cpp (which is what I added) and put in globals.cpp
 - I am now using "ExecuteCommand" instead of "system" to call the ibengine
executable ... so that we don't see the command line work when running through
the UI.
 - I added the "Customization is in Progress" dialog back and into interpret.cpp
... minus the simulated progress bar.
 - I added a check to make sure that the variables "CustomizationList" and
"_NewConfigName" are the same before running the ibengine executable since there
seems to be a problem with this when trying to create a customization in "Default".

There are no changes to comp.cpp or new file engine.cpp so the patches for these
I attached earlier are still valid

Comment 11

[Shruti Vasudevan]

Comment on attachment 61114 [details] [diff] [review]
updated patch ... everything except comp.cpp and engine.cpp (which remain the same)

>? globals/comp.cpp
>? globals/comp.h
>? ib/engine.cpp
>Index: cckwiz/iniFiles/build_page1.ini
>===================================================================
>RCS file: /cvsroot/mozilla/cck/cckwiz/iniFiles/build_page1.ini,v
>retrieving revision 1.37
>diff -u -r1.37 build_page1.ini
>--- cckwiz/iniFiles/build_page1.ini	3 Oct 2001 00:00:00 -0000	1.37
>+++ cckwiz/iniFiles/build_page1.ini	10 Dec 2001 18:38:41 -0000
>@@ -50,7 +50,7 @@
> 
>
> 
>
> [Navigation Controls]
>
>-onNext=Message(Are you ready to build your customized installer? [Yes]/[No]);IBEngine.StartIB();Msg(Installer creation is complete. The build is in %Root%Configs\%CustomizationList%\Output)
>
>+onNext=Message(Are you ready to build your customized installer? [Yes]/[No]);RunIB();Msg(Installer creation is complete. The build is in %Root%Configs\%CustomizationList%\Output)
>
> Help=InstallerHelp.ini
>
> 
>
> [Image 1]
>
>@@ -93,7 +93,7 @@
> ;opt7=Description of the Import Utility
>
> ;opt8=Description of the Internet Setup
>
> ;opt9=Description of the RealPlayer 5.0
>
>-onInit=IBEngine.GenerateComponentList()
>
>+onInit=globals.GenerateComponentList()
>
> 
>
> [Widget 2]
>
> Type=GlobalText
>
>Index: driver/WizardMachine.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/driver/WizardMachine.cpp,v
>retrieving revision 1.58
>diff -u -r1.58 WizardMachine.cpp
>--- driver/WizardMachine.cpp	14 Nov 2001 00:23:06 -0000	1.58
>+++ driver/WizardMachine.cpp	10 Dec 2001 18:38:42 -0000
>@@ -1266,24 +1266,6 @@
> 	return firstNode;
> }
> 
>-CString CWizardMachineApp::GetModulePath()
>-{
>-	char currPath[MID_SIZE];
>-    int     i,numBytes; 
>- 
>-    // Get the path of the file that was executed 
>-    numBytes = GetModuleFileName(m_hInstance, currPath, MIN_SIZE);
>-
>-	// get the cmd path
>-	// Remove the filename from the path 
>-	for (i=numBytes-1;i >= 0 && currPath[i] != '\\';i--);
>-	// Terminate command line with 0 
>-	if (i >= 0) 
>-		currPath[i+1]= '\0';
>-
>-	return CString(currPath);
>-}
>-
> CString CWizardMachineApp::GetGlobalOptions(CString theName)
> {
> 	CString temp="";
>Index: driver/WizardMachine.h
>===================================================================
>RCS file: /cvsroot/mozilla/cck/driver/WizardMachine.h,v
>retrieving revision 1.16
>diff -u -r1.16 WizardMachine.h
>--- driver/WizardMachine.h	5 Nov 1999 00:36:13 -0000	1.16
>+++ driver/WizardMachine.h	10 Dec 2001 18:38:42 -0000
>@@ -69,7 +69,6 @@
> 	void CreateNewCache();
> 	BOOL IsLastNode(NODE* treeNode);
> 	BOOL IsFirstNode(NODE* treeNode);
>-	CString GetModulePath();
> 	CString GetGlobalOptions(CString theName);
> 	void BuildWidget(WIDGET* aWidget, CString iniSection, CString iniFile, int pageBaseIndex, BOOL readValue);
> //	void BuildHelpWidget(WIDGET* aWidget, CString iniSection, CString iniFile, int pageBaseIndex);
>Index: driver/interpret.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/driver/interpret.cpp,v
>retrieving revision 1.53
>diff -u -r1.53 interpret.cpp
>--- driver/interpret.cpp	5 Dec 2001 18:48:02 -0000	1.53
>+++ driver/interpret.cpp	10 Dec 2001 18:38:42 -0000
>@@ -874,7 +874,7 @@
> 						CString outputPath	= rootPath + "Configs\\" + configName + "\\Output";
> 						char deletePath[MAX_SIZE];
> 						strcpy(deletePath, outputPath);
>-						CallDLL("IBEngine", "EraseDirectory", deletePath, w);
>+						CallDLL("globals", "EraseDirectory", deletePath, w);

						Since you are no longer using
IBEngine dll,
						there is no need for CallDll.
You can call
						"Erasedirectory" directly.
> 					}
> 			}
> 			// change pre-set CD autorun option
>@@ -888,7 +888,7 @@
> 					CString outputPath	= rootPath + "Configs\\" + configName + "\\Output";
> 					char deletePath[MAX_SIZE];
> 					strcpy(deletePath, outputPath);
>-					CallDLL("IBEngine", "EraseDirectory", deletePath, w);
>+					CallDLL("globals", "EraseDirectory", deletePath, w);

					same here. There is no need for
calldll.
> 				}
> 			}
> 			else if (strcmp(pcmd, "WriteCache") ==0)
>@@ -1202,6 +1202,31 @@
> 							p2 = strchr(parms1, ',');
> 					}
> 				}
>+			}
>+			else if (strcmp(pcmd, "RunIB") == 0)
>+			{
>+				//Make sure _NewConfigName is defined
>+				if(GetGlobal("_NewConfigName") != GetGlobal("CustomizationList"))
>+					SetGlobal("_NewConfigName", GetGlobal("CustomizationList"));
>+
>+				//Create an updated .che
>+				theApp.CreateNewCache();
>+
>+			        CString exec_command = "ibengine.exe -c " + CachePath;
>+
>+				CNewDialog newprog;
>+				newprog.Create(IDD_NEW_DIALOG,NULL );
>+				newprog.ShowWindow(SW_SHOW);
>+				CWnd * dlg;
>+				CRect tmpRect = CRect(7,7,173,13);
>+				dlg = newprog.GetDlgItem(IDC_BASE_TEXT);
>+				CWnd* pwnd = newprog.GetDlgItem(IDD_NEW_DIALOG);
>+				newprog.SetWindowText("Progress");
>+				dlg->SetWindowText("         Customization is in Progress ... ");
>+
>+				ExecuteCommand((char *)(LPCTSTR) exec_command, SW_HIDE, INFINITE);
>+
>+				newprog.DestroyWindow();
> 			}
> 			else if (strcmp(pcmd, "ShowSection") == 0)
> 			{
>Index: globals/globals.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/globals/globals.cpp,v
>retrieving revision 1.7
>diff -u -r1.7 globals.cpp
>--- globals/globals.cpp	13 Jan 2000 22:10:48 -0000	1.7
>+++ globals/globals.cpp	10 Dec 2001 18:38:43 -0000
>@@ -137,3 +137,43 @@
> 	WaitForSingleObject(processInfo.hProcess, wait);
> }
> 
>+extern "C" __declspec(dllexport)
>+void EraseDirectory(CString sPath)
>+{
>+        CFileFind finder;
>+	CString  sFullPath = sPath + "\\*.*";
>+
>+        BOOL bWorking = finder.FindFile(sFullPath);
>+        while (bWorking)
>+        {
>+        	bWorking = finder.FindNextFile();
>+        	if (finder.IsDots()) continue;
>+        	if (finder.IsDirectory())
>+        	{
>+            		CString dirPath = finder.GetFilePath();
>+            		EraseDirectory(dirPath);
>+            		_rmdir(finder.GetFilePath());
>+            		continue;
>+         	}
>+         	_unlink( finder.GetFilePath() );
>+     	}
>+}
>+
>+__declspec(dllexport)
>+CString GetModulePath()
>+{
>+        char currPath[MID_SIZE];
>+        int     i,numBytes;
>+
>+        // Get the path of the file that was executed
>+        numBytes = GetModuleFileName(NULL, currPath, MIN_SIZE);
>+
>+        // get the cmd path
>+        // Remove the filename from the path
>+        for (i=numBytes-1;i >= 0 && currPath[i] != '\\';i--);
>+        // Terminate command line with 0
>+        if (i >= 0)
>+                currPath[i+1]= '\0';
>+
>+        return CString(currPath);
>+}
>Index: globals/globals.h
>===================================================================
>RCS file: /cvsroot/mozilla/cck/globals/globals.h,v
>retrieving revision 1.7
>diff -u -r1.7 globals.h
>--- globals/globals.h	13 Jan 2000 22:10:43 -0000	1.7
>+++ globals/globals.h	10 Dec 2001 18:38:43 -0000
>@@ -12,3 +12,5 @@
> extern "C" __declspec(dllimport) void CopyDir(CString from, CString to, LPCTSTR extension, int overwrite);
> extern "C" __declspec(dllexport) void ExecuteCommand(char *command, int showflag, DWORD wait);
> extern "C" __declspec(dllimport) int GetAttrib(CString theValue, char* attribArray[]);
>+extern "C" __declspec(dllimport) void EraseDirectory(CString sPath);
>+__declspec(dllimport) CString GetModulePath();
>Index: globals/makefile.win
>===================================================================
>RCS file: /cvsroot/mozilla/cck/globals/makefile.win,v
>retrieving revision 1.7
>diff -u -r1.7 makefile.win
>--- globals/makefile.win	26 Nov 2001 22:53:45 -0000	1.7
>+++ globals/makefile.win	10 Dec 2001 18:38:43 -0000
>@@ -31,6 +31,7 @@
> 
> OBJS=					\
> 	.\$(OBJDIR)\globals.obj		\
>+	.\$(OBJDIR)\comp.obj		\
> 	$(NULL)
> 
> LINCS=  $(LINCS) \
>@@ -70,8 +71,9 @@
> 
> include <$(DEPTH)\cck\InitDist.win>
> 
>-export:: $(DLLNAME).h
>-	$(MAKE_INSTALL) $(DLLNAME).h ..\include
>+export:: 
>+	$(MAKE_INSTALL) globals.h ..\include
>+	$(MAKE_INSTALL) comp.h ..\include
> 
> libs:: $(DLL)
> 	$(MAKE_INSTALL) .\$(OBJDIR)\$(DLLNAME).lib ..\lib
>Index: ib/ib.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/ib/ib.cpp,v
>retrieving revision 1.94
>diff -u -r1.94 ib.cpp
>--- ib/ib.cpp	29 Nov 2001 22:49:44 -0000	1.94
>+++ ib/ib.cpp	10 Dec 2001 18:38:43 -0000
>@@ -1235,28 +1235,6 @@
> 	return retflag;
> }
> 
>-extern "C" __declspec(dllexport)
>-void EraseDirectory(CString sPath)
>-{
>-	CFileFind finder;
>-    CString  sFullPath = sPath + "\\*.*";
>-
>-	BOOL bWorking = finder.FindFile(sFullPath);
>-	while (bWorking) 
>-    {
>-        bWorking = finder.FindNextFile();
>-        if (finder.IsDots()) continue;
>-        if (finder.IsDirectory()) 
>-        {
>-            CString dirPath = finder.GetFilePath();
>-            EraseDirectory(dirPath);
>-            _rmdir(finder.GetFilePath());
>-            continue; 
>-         }
>-         _unlink( finder.GetFilePath() );
>-     }
>-}
>-
> void CopyDirectory(CString source, CString dest, BOOL subdir)
> // Copy files in subdirectories if the subdir flag is set (equal to 1).
> {
>@@ -1386,15 +1364,60 @@
> 	AfxMessageBox("Not enough disk space. Required: "+requiredSpace+" bytes. Available: "+availableSpace+" bytes.", MB_OK);
> }
> 
>-extern "C" __declspec(dllexport)
>-int StartIB(CString parms, WIDGET *curWidget)
>+
>+
>+BOOL FillGlobalWidgetArray(CString file)
>+{
>+	char buffer[MAX_SIZE] = {'\0'};
>+	CString name = "";
>+	CString value = "";
>+
>+       	FILE *globs = fopen(file, "r");
>+	if (!globs)
>+		return FALSE;
>+
>+	while(!feof(globs))
>+	{
>+		while(fgets(buffer, MAX_SIZE, globs))
>+		{
>+			if (strstr(buffer, "="))
>+			{
>+				name =  CString(strtok(buffer,"="));
>+				value = CString(strtok(NULL,"="));
>+				value.TrimRight();
>+				if (value.Find("%") >= 0) {
>+					//We should never enter this condition via wizardmachine.exe 
>+					AfxMessageBox("The current .che file called: "+file+" contains some unfilled parameters."
>+							"These parameters will appear between two percent (%) signs such as %Root%"
>+							"Please replace these parameters with their appropriate values and restart"
>+							"the program", MB_OK);
>+			//		value=theInterpreter->replaceVars((char *) (LPCSTR) value,NULL);
>+				}
>+				
>+				WIDGET* w = SetGlobal(name, value);
>+				if (w)
>+					w->cached = TRUE;
>+			}
>+		}
>+	}
>+
>+	fclose(globs);
>+
>+	return TRUE;
>+}
>+
>+
>+
>+int StartIB(/*CString parms, WIDGET *curWidget*/)
> {
> 	char *fgetsrv;
> 	int rv = TRUE;
> 	char	olddir[1024];
> 	componentOrder =0;
>-	rootPath	= GetGlobal("Root");
>-	configName	= GetGlobal("CustomizationList");
>+//	rootPath	= GetGlobal("Root");
>+	rootPath	= GetModulePath();
>+	WIDGET *w	= SetGlobal("Root", rootPath);
>+	configName	= GetGlobal("_NewConfigName");
> 	configPath  = rootPath + "Configs\\" + configName;
> 	outputPath	= configPath + "\\Output";
> 	cdPath 		= configPath + "\\Output\\Core";
>@@ -1444,6 +1467,13 @@
> 		CopyFile(nscpxpiPath+"\\recommended.end",
> 			templinuxPath+"\\xpi\\recommended.end", FALSE);
> 	}
>+
>+//Creating necessary directories

Include error checks for _mkdir

>+	_mkdir(configPath);
>+	_mkdir(outputPath);

The following 2 _mkdirs are unnecessary
>+	_mkdir(cdPath);
>+	_mkdir(cdshellPath);
>+
> 	iniSrcPath	= nscpxpiPath + "\\config.ini";
> //Check for disk space before continuing
> 
>@@ -1566,16 +1596,6 @@
> 		iniDstPath = outputPath + "\\config.ini";
> 		xpiDstPath = outputPath;
> 	}
>-	CNewDialog newprog;
>-	newprog.Create(IDD_NEW_DIALOG,NULL );
>-	newprog.ShowWindow(SW_SHOW);
>-/////////////////////////////
>-	CWnd * dlg;
>-	CRect tmpRect = CRect(7,7,173,13);
>-	dlg = newprog.GetDlgItem(IDC_BASE_TEXT);
>-	CWnd* pwnd = newprog.GetDlgItem(IDD_NEW_DIALOG);
>-	dlg->SetWindowText("         Customization is in Progress");
>-	
> /////////////////////////////
> 	_mkdir((char *)(LPCTSTR) tempPath);
> 	_mkdir((char *)(LPCTSTR) workspacePath);
>@@ -1623,16 +1643,13 @@
> 		fclose(f);
> 	}
> 
>-	dlg->SetWindowText("         Customization is in Progress \n         |||||||||");
> 
> 	// Put all the extracted files back into their new XPI homes
> 	ReplaceJARFiles();
> 
>-	dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||");
> 
> 	ReplaceXPIFiles();
> 
>-	dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||||||");
> 
> 	// Copy remaining default installer files into config
> 	// preserving any existing files that we created already
>@@ -1701,30 +1718,23 @@
> 	}
> 
> 	// Didn't work...
>-	dlg->SetWindowText("         Customization is in Progress \n         |||||||||||||||||||||||||||");
> 
> 	if (linuxOption == "Linux")
> 	{
> 		LinuxInvisible();
>-		dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||||||||||||||||||||||||");
> 		AddThirdParty();
>-		dlg->SetWindowText("         Customization is in Progress \n         |||||||||||||||||||||||||||||||||||||||||||||");
> 		CreateLinuxInstaller();
>-		dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||||||||||||||||||||||||||||||||||||||||||");
> 	}
> 	else
> 	{
> 		invisible();
> 
>-		dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||||||||||||||||||||||||");
> 
> 		AddThirdParty();
> 		
>-		dlg->SetWindowText("         Customization is in Progress \n         |||||||||||||||||||||||||||||||||||||||||||||");
> 		
> 		ReplaceINIFile();
> 		
>-		dlg->SetWindowText("         Customization is in Progress \n         ||||||||||||||||||||||||||||||||||||||||||||||||||||||");
> 	}
> 	SetCurrentDirectory(olddir);
> 	CString TargetDir = GetGlobal("Root");
>@@ -1732,8 +1742,6 @@
> 	CString MozBrowser = GetBrowser();
> //	CreateShortcut(MozBrowser, TargetFile, "HelpLink", TargetDir, FALSE);
> 
>-	dlg->SetWindowText("         Customization is in Progress \n         |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||");
>-	newprog.DestroyWindow();
> 	EraseDirectory(tempPath);
> 	_chdir(configPath);
> 	_rmdir("Temp");
>Index: ib/ib.h
>===================================================================
>RCS file: /cvsroot/mozilla/cck/ib/ib.h,v
>retrieving revision 1.5
>diff -u -r1.5 ib.h
>--- ib/ib.h	4 Jun 2001 21:37:21 -0000	1.5
>+++ ib/ib.h	10 Dec 2001 18:38:44 -0000
>@@ -1,3 +1,5 @@
>+#define MID_SIZE 512
>+
> typedef struct s_xpi {
> 	CString xpiname;
> 	CString filename;
>@@ -7,6 +9,8 @@
> 	CString filename;
> 	} JAR;
> 
>+int StartIB(void);
>+BOOL FillGlobalWidgetArray(CString);
> void CreateRshell(void);
> void CreateHelpMenu(void);
> void CreateBuddyList(void);
>
>Index: ib/makefile.win
>===================================================================
>RCS file: /cvsroot/mozilla/cck/ib/makefile.win,v
>retrieving revision 1.14
>diff -u -r1.14 makefile.win
>--- ib/makefile.win	26 Nov 2001 22:55:07 -0000	1.14
>+++ ib/makefile.win	10 Dec 2001 18:38:44 -0000
>@@ -35,9 +35,9 @@
> 	.\$(OBJDIR)\NewDialog.obj \
> 	.\$(OBJDIR)\wizshell.obj \
> 	.\$(OBJDIR)\ib.obj		\
>-	.\$(OBJDIR)\comp.obj	\
> 	.\$(OBJDIR)\Helpmenu.obj	\
> 	.\$(OBJDIR)\buddylist.obj	\
>+	.\$(OBJDIR)\engine.obj	\
> 	$(NULL)
> 
> #		-I$(DEPTH)\xpinstall\wizard\windows\setup \
>@@ -47,9 +47,13 @@
> 		-I$(DIST)\$(OBJDIR)\include \
> 		$(NULL)
> 
>-MAKE_OBJ_TYPE = DLL
>-DLLNAME = ibengine
>-DLL=.\$(OBJDIR)\$(DLLNAME).dll
>+MAKE_OBJ_TYPE = EXE 
>+EXENAME = ibengine
>+MAPFILE = ib.map
>+RESFILE = ib.res
>+PDBFILE = $(EXENAME)
>+
>+PROGRAM=.\$(OBJDIR)\$(EXENAME).exe
> 
> CFLAGS = \
> 	-W3 \
>@@ -80,14 +84,16 @@
> 
> include <$(DEPTH)\cck\InitDist.win>
> 
>-libs:: $(DLL)
>-	$(MAKE_INSTALL) .\$(OBJDIR)\$(DLLNAME).dll $(CCKDIST)\CCKTool
>-	$(MAKE_INSTALL) .\$(OBJDIR)\$(DLLNAME).lib ..\lib
>+libs:: $(PROGRAM)
>+	$(MAKE_INSTALL) $(PROGRAM) $(CCKDIST)\CCKTool
>+	$(MAKE_INSTALL) .\$(OBJDIR)\$(EXENAME).lib ..\lib
> 
> export::
> 	$(MAKE_INSTALL) script.ib $(CCKDIST)\CCKTool
> 	$(MAKE_INSTALL) help1.txt $(CCKDIST)\CCKTool
> 	$(MAKE_INSTALL) help2.txt $(CCKDIST)\CCKTool
>+
>+$(PROGRAM):: $(OBJDIR) $(OBJS) $(RESFILE)
> 
> #clobber::
> #	rm -f $(DIST)\bin\$(DLLNAME).dll

Comment 12

[Shruti Vasudevan]

Oops, I forgot to delete all the corect ones. To go thro the above comments in
an easier way, just refer to the foll. files in the above comment:
interpret.cpp, ib.cpp

Comment 13

[Shruti Vasudevan]

Pls. ignore my above 2 comments. I'll submit the comments on the patch again.

Comment 14

[Shruti Vasudevan]

Comment on attachment 61114 [details] [diff] [review]
updated patch ... everything except comp.cpp and engine.cpp (which remain the same)

>Index: driver/interpret.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/driver/interpret.cpp,v
>retrieving revision 1.53
>diff -u -r1.53 interpret.cpp
>--- driver/interpret.cpp	5 Dec 2001 18:48:02 -0000	1.53
>+++ driver/interpret.cpp	10 Dec 2001 18:38:42 -0000
>@@ -874,7 +874,7 @@
> 						CString outputPath	= rootPath + "Configs\\" + configName + "\\Output";
> 						char deletePath[MAX_SIZE];
> 						strcpy(deletePath, outputPath);
>-						CallDLL("IBEngine", "EraseDirectory", deletePath, w);

Since you are no longer using IBEngine dll, there is no need for CallDll.
You can call "Erasedirectory" directly.

>+						CallDLL("globals", "EraseDirectory", deletePath, w);
> 					}
> 			}
> 			// change pre-set CD autorun option
>@@ -888,7 +888,7 @@
> 					CString outputPath	= rootPath + "Configs\\" + configName + "\\Output";
> 					char deletePath[MAX_SIZE];
> 					strcpy(deletePath, outputPath);
>-					CallDLL("IBEngine", "EraseDirectory", deletePath, w);

Same here. There is no need for CallDLL.

>+					CallDLL("globals", "EraseDirectory", deletePath, w);
> 				}
> 			}


>Index: ib/ib.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/ib/ib.cpp,v
>retrieving revision 1.94
>diff -u -r1.94 ib.cpp
>--- ib/ib.cpp	29 Nov 2001 22:49:44 -0000	1.94
>+++ ib/ib.cpp	10 Dec 2001 18:38:43 -0000
>@@ -1235,28 +1235,6 @@
> 	return retflag;
> }

>+int StartIB(/*CString parms, WIDGET *curWidget*/)
> {
> 	char *fgetsrv;
> 	int rv = TRUE;
> 	char	olddir[1024];
> 	componentOrder =0;
>-	rootPath	= GetGlobal("Root");
>-	configName	= GetGlobal("CustomizationList");
>+//	rootPath	= GetGlobal("Root");
>+	rootPath	= GetModulePath();
>+	WIDGET *w	= SetGlobal("Root", rootPath);
>+	configName	= GetGlobal("_NewConfigName");
> 	configPath  = rootPath + "Configs\\" + configName;
> 	outputPath	= configPath + "\\Output";
> 	cdPath 		= configPath + "\\Output\\Core";
>@@ -1444,6 +1467,13 @@
> 		CopyFile(nscpxpiPath+"\\recommended.end",
> 			templinuxPath+"\\xpi\\recommended.end", FALSE);
> 	}
>+
>+//Creating necessary directories

Include error checks for _mkdir

>+	_mkdir(configPath);
>+	_mkdir(outputPath);

The following 2 _mkdirs are unnecessary

>+	_mkdir(cdPath);
>+	_mkdir(cdshellPath);
>+
> 	iniSrcPath	= nscpxpiPath + "\\config.ini";
> //Check for disk space before continuing

Comment 15

[Shruti Vasudevan]

Some minor bugs after applying the above patch:
1. Create a new config choosing CD autorun option and build installer. Revisit
the same config and uncheck the CD autorun option, build installer. "Core" dir
is still present in the config.

2. When we include CD autorun option and create a new config using "ibengine.exe
-c ...." the following files are not created in "output" dir: setup.exe,
launch.in, autorun.inf
Solution: Once you create dir "output", also create a file "DeleteThisFile" in
output dir.

3. Since we now have ibengine.exe instead of ibengine.dll, we may not be able to
call some generic functions in ib.cpp from UI specific files. I think we should
include some functions  of ib.cpp like CopyDirectory, diskspacealert, etc. in
globals.cpp. Not sure though, Steve, what do you think?

4. Progress bar (Shannon has already pointed out this)
The progress dialog does not have "||||||" lines to indicate progress. I have no
preference for this and it is fine with me to include it or exclude it. Lets see
what the others think. May be we can open a bug for this to get others opinion.

Note:
Pls. include a phrase or 1 line comment wherever appropriate in your patch.

Comment 16

[Shruti Vasudevan]

Comment on attachment 60690 [details]
new file engine.cpp

May be we should include keep track for the appropriate number of arguments.

>int main(int argc, char *argv[])
>{
>  CString configPath;
>  for(int i=0; i < argc; i++)
>  {
>    if(!strcmp(argv[i], "-c"))
>    {
>      configPath = argv[i+1];
>    }
>  }
>  BOOL result = FillGlobalWidgetArray(configPath); 
>  int returnit = StartIB();

Are we using result or returnit anywhere else in the program?
If not, I assume we are using it to find if FillGlobalWidgetArray
and StartIB were successful. But, I don't see any verification once 
result and returnit get some values, they seem like extra variables.

>  printf("\nFinished StartIB!!\n");
>  return 0;
>}

Comment 17

[Steve Meredith (gone)]

I pulled and built and added Factory GUI. I haven't seen any run-time problems
so far. I'll take a look at the code next.

If you run ibengine with no arguments, you get an assertion. You might check the
command line params and print a help message as needed.

>3. Since we now have ibengine.exe instead of ibengine.dll, we may not be able to
>call some generic functions in ib.cpp from UI specific files. I think we should
>include some functions  of ib.cpp like CopyDirectory, diskspacealert, etc. in
>globals.cpp. Not sure though, Steve, what do you think?

I'd say move them over on an as-needed basis. 

>4. Progress bar (Shannon has already pointed out this)
>The progress dialog does not have "||||||" lines to indicate progress. I have no
>preference for this and it is fine with me to include it or exclude it. Lets see
>what the others think. May be we can open a bug for this to get others opinion.

We don't need those lines.

Comment 18

[Steve Meredith (gone)]

ExecuteCommand() doesn't do any error checking when it calls external programs.
Since we are adding yet another call to an external program, and especially
since ibengine doesn't display errors itself, perhaps this is the time to fix
ExecuteCommand(). See bug 102537 as a related bug.

Comment 19

[Shannon Diener]

Attachment: diff (from CCK_IB_BRANCH) to fix some problems mentioned

Comment 20

[Shannon Diener]

The patch I just attached (which is a diff against my CCK_IB_BRANCH) should
address some of the concerns mentioned:

 - EraseDirectory is now called directly in interpret.cpp
 - extra variables in engine.cpp have been removed
 - Created another command line switch so that wizardmachine will use the "-c"
switch while the user will use the "-u" switch ... I did this because there are
some functions I want to run only if the user is running via command line.
 - The bugs Shruti found with CD Autorun were caused because I forgot to copy
over the files and directories under WSTemplate ... this is fixed in engine.cpp
 - inserted a printf for a user running ibengine.exe via command line who does
not supply the correct switch (currently "-u").

Problems that still exist:  ExecuteCommand() needs to do error checking 

Comment 21

[Shruti Vasudevan]

Comment on attachment 61318 [details] [diff] [review]
diff (from CCK_IB_BRANCH) to fix some problems mentioned

Thanks for updating the patch with the changes. Some minor comments:

>Index: ib/engine.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/cck/ib/Attic/engine.cpp,v
>retrieving revision 1.1.2.1
>diff -u -r1.1.2.1 engine.cpp
>--- ib/engine.cpp	10 Dec 2001 22:26:59 -0000	1.1.2.1
>+++ ib/engine.cpp	11 Dec 2001 20:45:07 -0000
>@@ -13,15 +13,61 @@

From the patch below, it looks like in all cases the no. of arguments should be
3. 
So, first check if the no. of arguments are correct. And then, check if argv[1]
has the right option.

> int main(int argc, char *argv[])
> {
>   CString configPath;
>-  for(int i=0; i < argc; i++)
>+  CString rootPath;
>+  CString templateDir;
>+  CString configname;
>+  CString configpath;
>+  CString che_path;
>+  CString che_file;
>+  
>+  if(!strcmp(argv[1], "-c"))
>   {
>-    if(!strcmp(argv[i], "-c"))
>-    {
>-      configPath = argv[i+1];
>-    }
>+    //The option "-c" means that ibengine.exe was called from wizardmachine.exe
>+    configPath = argv[2];
>+    FillGlobalWidgetArray(configPath);
>   }
>-  BOOL result = FillGlobalWidgetArray(configPath); 
>-  int returnit = StartIB();
>-  printf("\nFinished StartIB!!\n");
>+  else if(!strcmp(argv[1], "-u"))
>+  {
>+    //The option "-u" means that the user is running ibengine.exe via command line
>+    configPath = argv[2];
>+    FillGlobalWidgetArray(configPath);
>+    rootPath = GetModulePath();
>+    templateDir = rootPath + "WSTemplate";
>+    configname = GetGlobal("_NewConfigName");
>+    configpath = rootPath + "Configs\\" + configname;
>+
>+    //Grab exact name of the .che file from configPath
>+    che_file = configPath;
>+    int extractposition = che_file.ReverseFind('\\');
>+    extractposition++;
>+    extractposition = (che_file.GetLength()) - extractposition;
>+    che_file = che_file.Right(extractposition);
>+
>+    //These are some commands that we only want to run if ibengine.exe is run at command line
>+
>+    //Create the config path
>+    _mkdir(configpath);
>+
>+    //Copy files and directories from WSTemplate to new config directory
>+    CopyDir(templateDir, configpath, NULL, FALSE);
>+
>+    //Copy the .che file given on command line to the appropriate directory
>+    che_path = configpath + "\\" + che_file;
>+    CopyFile(configPath, che_path, FALSE);
>+  }
>+  else
>+  {
>+    printf("\nYou have supplied incorrect command line options. \n"
>+           "Please, run either \"WizardMachine.exe\" or \"ibengine.exe -u <your_config_file_path>\"\n");
>+    return 1;
>+  } 
>+      
>+
>+  StartIB();
>+
>+  CString root = GetGlobal("Root");
>+  CString configName = GetGlobal("_NewConfigName");
>+

Avoid the above 2 statements and use rootPath and configname in the printf
statement.

>+  printf("\nInstaller creation is complete. The build is in %sConfigs\\%s\\Output\n", root, configName);

"root" and "configName" are CStrings. Do they work in printf? I think you have
to
first convert them to char *.

>   return 0;
> }

Comment 22

[Shruti Vasudevan]

Comment on attachment 61318 [details] [diff] [review]
diff (from CCK_IB_BRANCH) to fix some problems mentioned

>+    //Create the config path
>+    _mkdir(configpath);
>+

There is still no error checking for _mkdir. But, you don't have to fix it now.
Just open a bug for this. See relevant bug 106272.

Comment 23

[Shannon Diener]

Attachment: new diff

Comment 24

[Shannon Diener]

The patch above is a diff against the CCK_IB_BRANCH ... I marked obsolete all
previous patches that have already been checked into this branch.

This patches addresses Shruti's comments:

 - There is now a check for the number of arguments
 - The duplicated variables have been removed by moving the call to StartIB()
and the printf into their appropriate conditions

I retested the use of CString in printf statements and it seems to work correctly.

This patch does not include error checking for _mkdir but, as Shruti mentioned,
there is currently a bug for this. So, once we establish how the error checking
will be done for other _mkdir calls I will apply it to this one.

Shruti, please let me know if, with the addition of this patch, you have any
additional concerns about the changes in the CCK_IB_BRANCH.  Thanks!

Comment 25

[Shruti Vasudevan]

Comment on attachment 61479 [details] [diff] [review]
new diff 

>+  if(argc == 3)
>   {
Include here:
      configPath = argv[2];
      FillGlobalWidgetArray(configPath);
This will avoid dupliication.

>+    if(!strcmp(argv[1], "-c"))
>+    {
>+      //The option "-c" means that ibengine.exe was called from wizardmachine.exe

>+      configPath = argv[2];
>+      FillGlobalWidgetArray(configPath);
remove the above 2 lines.

>+      StartIB();
>+    }
>+    else if(!strcmp(argv[1], "-u"))
>+    {
>+      //The option "-u" means that the user is running ibengine.exe via command line
>+      configPath = argv[2];
>+      FillGlobalWidgetArray(configPath);
remove the above 2 lines.

>+      rootPath = GetModulePath();

Note: As far as possible, limit the line length of code and comments to 80
chars. Sorry, for not mentioning this before, I realized it now.

Comment 26

[Shannon Diener]

Attachment: diff replacing previous patch including Shruti's suggestions

Comment 27

[Rodney Velasco]

note regarding ibengine.exe...
shannon, Jimmy and I found out that ibengine.exe does not handle file names that
contain spaces.."C:\program files\netscape\blah", ibengine.exe fails to run
because of the space between "Program" and "files".  Shannon's working on a fix.

Comment 28

[tao]

Shruti ran into this in 6.2 time. Shruti - how did you fix it?

Comment 29

[tao]

I believe we escaped the space. Jimmy - do you remember the bug number? We can
look at the patch to save some try/error.

Comment 30

[Shruti Vasudevan]

> Shruti ran into this in 6.2 time. Shruti - how did you fix it?
Just include the path within quotes.

Comment 31

[Shannon Diener]

http://bugzilla.mozilla.org/show_bug.cgi?id=100204

I think this was the bug mentioned earlier ... 

I'm working on a patch now

Comment 32

[Shannon Diener]

Attachment: Adding quotes around path

Comment 33

[Shannon Diener]

This will fix the problem when running the UI and we'll instruct the user to
include quotes around the path when running via command line.

Comment 34

[Rodney Velasco]

Bom-shik ran CCK acceptance tests on shannond's branch build.  No breakage of
CCK found after testing.  Changes can be checked into trunk.

Comment 35

[Shannon Diener]

CCK_IB_BRANCH has been merged into the trunk.  Resolving fixed

Comment 36

[blee]

FYI, Basic Feature test didn't include testing Linux bits.

Comment 37

[blee]

Just ran the same basic feature test with Linux 12-14-21-trunk bld --> Passed.

Comment 38

[Rodney Velasco]

This work has already been completed and tested on the trunk.  Marking this
verified.