On Sat, 6 Sep 2003, Stas Bekman wrote: > Randy Kobes wrote: > > On Thu, 4 Sep 2003, Stas Bekman wrote: > > > > > >>In the effort to remove some of the Win32 noise, I was > >>thinking that we can write a generic function which gets a > >>path as an argument and figures out internally if it needs > >>to keep the argument as passed or mangle it. So it'll do > >>something like: > >> > >> my $cwd = Apache::TestUtil::path(cwd); > >> > >>probably need a more intuitive name for this function. > > > > > > That'd be nice - a version that does this appears below. > > I named it win32_long_path - it'll just return what was > > passed into it if not on Win32. > > But my idea was to eliminate any os-specific words win32. I think it should > just be long_path... Think of it as File::Spec function.
OK, I'll do that. I put in the win32_ to indicate that it'll do something for Win32, and otherwise just return what was passed in. > I'm still not quite happy about the naming of the function, what exactly > GetLongPathName($file) does? > can this be done using some File::Spec function? I haven't found an equivalent one in File::Spec ... The problem here is that cwd (or any file/directory name) on Win32 has two representations - a long path name (that can include spaces) and a short path name, limited to 6 characters (this is a holdout from early days), plus 3 if an extension is present. Here we want to match cwd to /Apache-Test/, but cwd may return the short path name (eg, Apache~1.0), depending on if, for example, you have different Apache-Test* directories at the same level. So GetLongPathName can be used to get the full long path name. The thing with naming it, eg, just long_path, is that Unix users (the majority) wouldn't know what it does. > > +require File::Spec; > > > + my $candidate = File::Spec->catfile($_, 'Apache', $file); > > we import catfile in all other Apache::Test files, can do > that here as well... will make code simpler Good point - thanks. > > > + if (-e $candidate) { > > + $sys_config = $candidate; > > + last; > > + } > > + } > > + if ($sys_config) { > > + eval {require $sys_config}; > > + return $sys_config if (not $@ and IN_APACHE_TEST); > > + $sys_config = undef if $@; > > + } > > Hmm, I thought we agreed that eval {require $sys_config} will always succeed, > since that file is always available. so this code should be needed: > > + die 'Could not find a suitable Apache::TestConfigData' > + unless defined CONFIG_DATA; I was thinking here if someone, after installation, had mis-edited Apache::TestConfigData, either the system one or one found in some path in @INC being added thru, eg, use lib). But this might be overkill - not worrying about this at this time will simplify things here. > One more problem is TestRun.pm's layout: subs should go > after the declarations (uses/constants/etc). use 'use > subs' if you need to predeclare them. OK - thanks. > Another issue, is in_apache_test. Since a user may get it > with modperl-2.0 or separately it should return true in > either case. I forgot about that - thanks. > > +sub write_config { > > + my $self = shift; > > + my $fh = Symbol::gensym(); > > + my $vars = $self->{test_config}->{vars}; > > + my $conf_opts = $self->{conf_opts}; > > + my $file = IN_APACHE_TEST ? > > + catfile($vars->{top_dir}, CONFIG_DATA) : > > + CONFIG_DATA; > > it's easier to parse when written as: > > my $file = IN_APACHE_TEST > ? catfile($vars->{top_dir}, CONFIG_DATA) > : CONFIG_DATA; > > > + my $pkg = << "EOC"; > > +package Apache::TestConfigData; > > better have it strict, to avoid misterious errors (same in the pod below) > > use strict; > use warnings; > > use vars (\$Apache::TestConfigData); Thanks - I'll add that. I was also thinking about the problem of $ENV{HOME} not being available in mod_perl. As Apache::TestConfigData is being loaded in order to configure things, shouldn't this not be a problem, because at this point one isn't yet in a mod_perl environment? -- best regards, randy