Apache OpenOffice (AOO) Bugzilla – Issue 43252
[PATCH] Use $(MAKECMD:d)\startup if $(DMAKEROOT) is not set
Last modified: 2013-08-07 15:34:52 UTC
I've built dmake.exe, copied it and its startup/ directory to C:\dmake, and put C:\dmake on my %PATH%. When I try to run dmake.exe from some other directory, say, C:\Temp, I find that it is unable to run because it can't locate its startup files. Reading the manual I see that there is a new DMAKEROOT environment variable (well, new to me, anyway -- my old dmake 4.1 didn't have it). It works fine if I set DMAKEROOT=C:\dmake\startup, but I think it would be nice if dmake.exe defaulted to using the $(MAKECMD:d)\startup (which I believe is what dmake 4.1 used to do) if DMAKEROOT is not set. The patch below achieves this for MS VC++ builds by making DMAKEROOT default to $(MAKECMD:d)\startup. Thus, dmake 4.1 users can carry on working as they are used to (with no DMAKEROOT), while newer users who have DMAKEROOT set will not be affected. Note that I had to insert a call to the Win32 API function GetModuleFileName() to do this because argv[0] is not set to a full path when built with VC++ unless dmake.exe was actually invoked with the full path (i.e. the user typed "C:\dmake\dmake.exe"). I've a hunch that Borland C++ sets argv[0] to the full path anyway, so such games may not be necessary for Borland. I've no idea what would need doing to effect the same change for other OS's, or even if they honour DMAKEROOT to start with, but I'd really like to have this change on Win32 at least. diff -ruN dmake.orig/sysintf.c dmake/sysintf.c --- dmake.orig/sysintf.c 2004-10-22 09:05:44.000000000 +0100 +++ dmake/sysintf.c 2005-02-21 11:48:24.000000000 +0000 @@ -59,6 +59,12 @@ -- Use cvs log to obtain detailed change logs. */ +#ifdef _MSC_VER +# define WIN32_LEAN_AND_MEAN +# include <windows.h> +# include <winbase.h> +#endif + #include "extern.h" #include "sysintf.h" @@ -376,6 +382,12 @@ int argc; char* argv[]; { +#ifdef _MSC_VER + /* MS VC++ doesn't set argv[0] to the full path, so do it now. */ + static char szModuleName[_MAX_PATH]; + GetModuleFileName(NULL, szModuleName, sizeof(szModuleName)); + argv[0] = szModuleName; +#endif Pname = (argc == 0) ? DEF_MAKE_PNAME : argv[0]; Root = Def_cell( ".ROOT" ); Targets = Def_cell( ".TARGETS" ); diff -ruN dmake.orig/win95/microsft/config.h dmake/win95/microsft/config.h --- dmake.orig/win95/microsft/config.h 2004-10-22 09:11:52.000000000 +0100 +++ dmake/win95/microsft/config.h 2005-02-21 11:50:40.000000000 +0000 @@ -79,8 +79,10 @@ # define SIGQUIT SIGTERM #endif -/* MSC doesn't seem to care about CONST */ -#define CONST +/* MSC didn't seem to care about CONST in the past */ +#ifndef CONST +# define CONST +#endif /* MSC has sys/types.h and sys/stat.h (this is tested only with MSVC++ 6.0) */ #define HAVE_SYS_TYPES_H 1 diff -ruN dmake.orig/win95/startup.h dmake/win95/startup.h --- dmake.orig/win95/startup.h 2000-09-22 16:33:36.000000000 +0100 +++ dmake/win95/startup.h 2005-02-21 12:36:18.040278100 +0000 @@ -24,5 +24,5 @@ -- Use cvs log to obtain detailed change logs. */ -/*"MAKESTARTUP := $(MAKECMD:d)startup/startup.mk",*/ +"DMAKEROOT *= $(MAKECMD:d)startup", "MAKESTARTUP := $(DMAKEROOT)\\startup.mk", End of Patch.
Created attachment 22874 [details] Patch file, in case the inlined version doesn't apply.
please have a look
The +++ dmake/win95/startup.h 2005-02-21 12:36:18.040278100 +0000 ... +"DMAKEROOT *= $(MAKECMD:d)startup", part makes sense for an environment without DMAKEROOT set, but your patch contains a lot of other changes that are not motivated here. I will commit something like the line above for the next dmake CWS.
I disagree that "the other changes are not motivated here". The other changes may or may not be the best way to achieve what is required, but what they achieve definitely *is* required. These other changes ensure that argv[0] contains the full path of the dmake.exe executable, without which the $(MAKECMD:d) construct is useless because it won't necessarily contain a directory component. To demonstrate the problem, try building the following program with VC++ (I'm using version 6.0): #include <stdio.h> void main(int argc, char **argv) { printf("argv[0]=%s\n", argv[0]); } If I create a program called test.exe from this and then invoke it simply as "test" then the output is "argv[0]=test", i.e. argv[0] doesn't contain a directory component. In general, argv[0] seems to contain exactly the string that was used to invoke the program, rather than the fully qualified path of the program: C:\Temp>test argv[0]=test C:\Temp>test.exe argv[0]=test.exe C:\Temp>C:\Temp\test argv[0]=C:\Temp\test C:\Temp>C:\Temp\test.exe argv[0]=C:\Temp\test.exe Unless this issue is addressed somehow, then the $(MAKECMD:d) construct is useless unless the user happens to invoke dmake via its fully qualified path, which is not how I work, at least -- I invariably add the path that dmake.exe is in to my %PATH% environment variable and simply invoke dmake as "dmake".
@shay: You see, comments help ;) I confess I didn't test the patch yet, I only looked at the source. Thanks for explaining. A comment that this is needed for MAKECMD should be added.
The original patch already contains this comment: /* MS VC++ doesn't set argv[0] to the full path, so do it now. */ Do you want a more verbose comment than this?
Yes, I wanted to have a coment about the why, not the what. I.e. used for MAKECMD but in the meantime I checked how other (non MSVC targets) handle this case. I'm opposed to alter the meaning of MAKECMD (and even worse only for one target) because it simply is the command (without parameters) that is given to start the program. The "other" targets at the moment predefine DMAKEROOT to DMAKEROOT := '$prefix'/share/startup at compile time. This is obviously not possible for the MSVC build as it doesn't use the *nix like "configure" approach. I propose the following, for MSVC we define the value of DMAKEROOT to use the directoryname of GetModuleFileName. This is a bit tricky, and has to be done in inmacs.c to define DMAKEROOT as a new internal macro (MSVC only). And the documentation has to be updated.
[Reply to an email, to keep the discussion recorded here.] vq@openoffice.org wrote: > >What I wrote there is possible, but I looked into methods getting the >absolute path for several OSs. MSVC and Linux are possible, but there >doesn't seem to be a general solution for this problem. I liked your suggestion of setting DMAKEROOT to the directoryname of GetModuleFileName on Win32. I agree that it was better than my suggestion of changing the meaning of MAKECMD. But why are you trying to trying to come up with a general solution to the absolute path problem? I thought it was only Win32 that would be needing it. Other targets using a "configure" approach already have a sensible default for DMAKEROOT. Or are there targets other than Win32 which also don't use "configure"? > >Do you really think it is necessary to introduce the feature that dmake >(only the MSVC version) earches for its startup file in the directory >where its executable lives? And break the cross-platform compatibility? I believe that dmake on Win32 should search for startup files in the startup/ sub-directory of where the executable lives, i.e. that DMAKEROOT should default to that directory. I don't see that this is breaking cross-platform compatibility, because we don't currently have cross-platform compatibility anyway -- all targets currently look for startup files in DMAKEROOT (and this would not be changed), but on targets built via "configure", DMAKEROOT defaults to a compile-time choice of directory, while on Win32 DMAKEROOT currently has no default. All I'm asking is that Win32 gets a sensible default for DMAKEROOT too. The startup/ sub-directory of where the executable is seems like a sensible choice since there is no $prefix directory specified at compile-time. It is also what Perl users of the old 4.10pl1 version are used to ;) > >On could also set the internal DMAKEROOT at compile time like it is done >for the configure based dmake builds. (But not for the MSVC build.) > >Is setting the MAKESTARTUP or DMAKEROOT environment not enough to make >the users happy? ;) No, it isn't. On Unix it's OK because dmake looks for startup files in a sensible place, specified at compile time. You only need to set DMAKEROOT if you want to change the default. But on Win32, dmake currently looks for "\startup.mk", which is absurd. Therefore, users *have* to set DMAKEROOT, which isn't very friendly when (a) the startup files are almost certainly in the startup/ sub-directory of where the executable is, and (b) dmake.exe is perfectly capable of looking there.
>> Is setting the MAKESTARTUP or DMAKEROOT environment not enough to make >> the users happy? ;) > > No, it isn't. On Unix it's OK because dmake looks for startup files in a > sensible place, specified at compile time. You only need to set DMAKEROOT if > you want to change the default. But on Win32, dmake currently looks for > "\startup.mk", which is absurd. I would call it a bug ;) One could set DMAKEROOT to something that is defined at compile time too. > Therefore, users *have* to set DMAKEROOT, which > isn't very friendly when (a) the startup files are almost certainly in the > startup/ sub-directory of where the executable is, and (b) dmake.exe is > perfectly capable of looking there. IMHO capability is not the problem here, but that Windows is lacking something like a default directory (that is everywhere the same), where programm packages are installed in, is propably good reason. ( Another reason is that I have a patch ready to accomplish this ;) ) The following patch a macro ABSMAKECMD which contains the full pathname to the current dmake programm for native Windows dmake (MSVC and MinGW) and "" for all other compilers and sets "DMAKEROOT *= $(ABSMAKECMD:d)startup". I verified that it builds/works MinGW but the patch should be OK for MSVC6 too. Can you check that? I'll test later with VS .NET2003. But we definitely have to update/change the documentation to mention this special Windows treatment.
I don't see any patch yet. Did you omit it, or am I being impatient?
Created attachment 23655 [details] Patch to add ABSMAKECMD
Are you sure that's the right patch? :-s
No, it's not. Sorry. Next try ....
Created attachment 23656 [details] Patch to add ABSMAKECMD (I hope)
Works fine for me. Compiled with no errors using MSVC++ 6.0. Testing with a makefile that simply echoes $(MAKECMD) and $(ABSMAKECMD) I get the expected values for each with various ways of invoking dmake; and DMAKEROOT still works too. Thanks.
Created attachment 24904 [details] Patch for dmake/
Committed the previous patch to cws_src680_dmake43p01. vq->hjs: Please verify.
Set to fixed ..
I see that the "verify" request was directed at hjs, but I just thought I'd confirm that this is working fine for me.
seems to do what is proposed in the issue..
.