[Fedora-directory-devel] cleaning up FILE_PATHSEP?

[Andrew Bartlett]

I recently began work on bug 239765, and came across the wonder of
FILE_PATHSEP:

#ifdef XP_UNIX

#define FILE_PATHSEP  '/'
#define FILE_PATHSEPP "/"
#define FILE_PARENT   "../"
#define WSACleanup()

#elif defined(XP_WIN32)

#define FILE_PATHSEP  '/'
#define FILE_PATHSEPP "\\"
#define FILE_PARENT   "..\"

#endif /* XP_WIN32 */

Firstly, what little I knew about windows file handling told me that we
could, at least on the supply side, use a unix /.

Indeed, I noted that much of create_instance does exactly that *and*
passes in FILE_PATHSEP via %c.  If we needed FILE_PATHSEP, then this
would already be a problem:

/* generate <confdir>/slapd-collations.conf */
    PR_snprintf(src, sizeof(src), "%s%c%s%c/config/%s-collations.conf",
                cf->sysconfdir, FILE_PATHSEP, cf->package_name,
                FILE_PATHSEP, PRODUCT_NAME);
    PR_snprintf(dest, sizeof(dest), "%s%c%s-collations.conf",
            cf->config_dir, FILE_PATHSEP, PRODUCT_NAME);

Even more priceless is the use of FILE_PATHSEP in
ldap/admin/src/configure_instance.cpp:create_console_script().  (is
there a /bin/sh that doesn't accept / as a path?).

Anyway, for my own amusement, I'll post a patch (probably untested on
win32) to make this a little easier on the eyes, and perhaps even easier
on new programmers to the codebase.

Thoughts?

Andrew Bartlett

[Rich Megginson]

Andrew Bartlett wrote:
> I recently began work on bug 239765, and came across the wonder of
> FILE_PATHSEP:
>
> #ifdef XP_UNIX
>
> #define FILE_PATHSEP  '/'
> #define FILE_PATHSEPP "/"
> #define FILE_PARENT   "../"
> #define WSACleanup()
>
> #elif defined(XP_WIN32)
>
> #define FILE_PATHSEP  '/'
> #define FILE_PATHSEPP "\\"
> #define FILE_PARENT   "..\"
>
> #endif /* XP_WIN32 */
>
> Firstly, what little I knew about windows file handling told me that we
> could, at least on the supply side, use a unix /.

Yes.  For modern versions of Windows.  This may have been an artifact of 
Win95/98/NT, or perhaps cases where these paths were passed to a command 
interpreter (e.g. in a system() call).
> Indeed, I noted that much of create_instance does exactly that *and*
> passes in FILE_PATHSEP via %c.  If we needed FILE_PATHSEP, then this
> would already be a problem:
> /* generate <confdir>/slapd-collations.conf */
> PR_snprintf(src, sizeof(src), "%s%c%s%c/config/%s-collations.conf",
>             cf->sysconfdir, FILE_PATHSEP, cf->package_name,
>             FILE_PATHSEP, PRODUCT_NAME);
> PR_snprintf(dest, sizeof(dest), "%s%c%s-collations.conf",
>         cf->config_dir, FILE_PATHSEP, PRODUCT_NAME);

It probably doesn't matter anymore now that modern versions of Windows 
support both \ and /.
> Even more priceless is the use of FILE_PATHSEP in
> ldap/admin/src/configure_instance.cpp:create_console_script().  (is
> there a /bin/sh that doesn't accept / as a path?).

Probably not anymore, if there ever was one that had a problem.
> Anyway, for my own amusement, I'll post a patch (probably untested on
> win32) to make this a little easier on the eyes, and perhaps even easier
> on new programmers to the codebase.
>
> Thoughts?

FILE_PATHSEP is used throughout fedora ds code . . .
> Andrew Bartlett
>
> --
> Fedora-directory-devel mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/fedora-directory-devel

[Andrew Bartlett]

On Mon, 2007-05-21 at 07:49 -0600, Richard Megginson wrote:
> Andrew Bartlett wrote:
> > Anyway, for my own amusement, I'll post a patch (probably untested on
> > win32) to make this a little easier on the eyes, and perhaps even easier
> > on new programmers to the codebase.
> >
> > Thoughts?
>
> FILE_PATHSEP is used throughout fedora ds code . . .

Yes and no.  It seems confined mainly to the admin code (I presume the
rest is isolated by NSPR?).

Attached is the patch I made up, for create_instance at least.  For the
moment, It includes the schema work (because the two overlap), but I'll
redo this once the schema work is in the tree.

Some feedback from a windows system would be useful.

Andrew Bartlett

[Rob Crittenden]

Andrew Bartlett wrote:
> I recently began work on bug 239765, and came across the wonder of
> FILE_PATHSEP:
>
> #ifdef XP_UNIX
>
> #define FILE_PATHSEP  '/'
> #define FILE_PATHSEPP "/"
> #define FILE_PARENT   "../"
> #define WSACleanup()
>
> #elif defined(XP_WIN32)
>
> #define FILE_PATHSEP  '/'
> #define FILE_PATHSEPP "\\"
> #define FILE_PARENT   "..\"
>
> #endif /* XP_WIN32 */
>
> Firstly, what little I knew about windows file handling told me that we
> could, at least on the supply side, use a unix /.
>
> Indeed, I noted that much of create_instance does exactly that *and*
> passes in FILE_PATHSEP via %c.  If we needed FILE_PATHSEP, then this
> would already be a problem:
> /* generate <confdir>/slapd-collations.conf */
> PR_snprintf(src, sizeof(src), "%s%c%s%c/config/%s-collations.conf",
>             cf->sysconfdir, FILE_PATHSEP, cf->package_name,
>             FILE_PATHSEP, PRODUCT_NAME);
> PR_snprintf(dest, sizeof(dest), "%s%c%s-collations.conf",
>         cf->config_dir, FILE_PATHSEP, PRODUCT_NAME);
>
> Even more priceless is the use of FILE_PATHSEP in
> ldap/admin/src/configure_instance.cpp:create_console_script().  (is
> there a /bin/sh that doesn't accept / as a path?).
>
> Anyway, for my own amusement, I'll post a patch (probably untested on
> win32) to make this a little easier on the eyes, and perhaps even easier
> on new programmers to the codebase.
>
> Thoughts?
>
> Andrew Bartlett

DS used to include a significant portion Netscape Enterprise Server 
(NES) 3.0. Prior to open sourcing I removed several hundred unused files 
and hundreds more lines of unnecessary code. I suspect that this is 
NES-specific code that found its way into DS in a viral sort of way.

In any case, it is only the 2 P version that is really a problem, right? 
The code you included would do single-character replacements AFAICT.

Prior to open sourcing I spent more time than I'd like to admit on 
getting it to at least compile on win32.

On top of this there are separate re-definitions of this code, in 
configure_instance.cpp for example. That can probably be removed.

Now certainly one shouldn't mix FILE_PATHSEP with an explicit "/" in the 
same PR_snprintf. I think that there is less effort these days to 
support win32.

As for create_console_script(), I have a fairly old source tree but in 
mine the whole function is #if 0'd out and even then is only meant to 
run on unix-like systems.

Its important to remember that portitions of the DS code supported 
versions of Solaris back to 2.4, OSF1, Irix, HP/ux, AIX, Linux and 
Windows (and perhaps one or two more that have slipped my mine). You 
haven't lived until you've broken the build on 6 platforms :-) On my 
team the unlucky developer got the attached plungers displayed on top of 
their cube.

rob

[Andrew Bartlett]

On Mon, 2007-05-21 at 09:50 -0400, Rob Crittenden wrote:
> Its important to remember that portitions of the DS code supported 
> versions of Solaris back to 2.4, OSF1, Irix, HP/ux, AIX, Linux and 
> Windows (and perhaps one or two more that have slipped my mine). You 
> haven't lived until you've broken the build on 6 platforms :-) On my 
> team the unlucky developer got the attached plungers displayed on top of 
> their cube.

Yeah, this is why I put a lot of time and effort into Samba's automated
testing, and the real-time results on the build farm.  We get a lot of
value from that resource.

Any news/plans for a public build farm for Fedora DS?

Sadly for samba, the postage on the plungers would be prohibitive ;-)

Andrew Bartlett

[Rich Megginson]

Andrew Bartlett wrote:
> On Mon, 2007-05-21 at 09:50 -0400, Rob Crittenden wrote:
> > Its important to remember that portitions of the DS code supported 
> > versions of Solaris back to 2.4, OSF1, Irix, HP/ux, AIX, Linux and 
> > Windows (and perhaps one or two more that have slipped my mine). You 
> > haven't lived until you've broken the build on 6 platforms :-) On my 
> > team the unlucky developer got the attached plungers displayed on top of 
> > their cube.
>
> Yeah, this is why I put a lot of time and effort into Samba's automated
> testing, and the real-time results on the build farm.  We get a lot of
> value from that resource.
>
> Any news/plans for a public build farm for Fedora DS?

Yes, there are plans for a tinderbox-style environment (probably using 
buildbot instead of tinderbox, though).  I'll defer to those folks 
working on it for an update of the progress.
> Sadly for samba, the postage on the plungers would be prohibitive ;-)
>
> Andrew Bartlett
>
> --
> Fedora-directory-devel mailing list
> [e-mail]
> https://www.redhat.com/mailman/listinfo/fedora-directory-devel