Re: [Setup-tool-hackers] Updated FreeBSD patch

[Hans Petter Jansson <hpj ximian com>]

> New patch. Contains some changes to the frontend to make them compile, 
> tweaked my previous backend changes, fixed configure.in and consorts, added 
> an mtab-parse-function for freebsd and included ChangeLog entries.
> 
> Still todo: Use the new mtab-parse-thingy everywhere, fix the timezone 
> frontend.
> 
> Patch attached.

Even big patches are okay to attach as plaintext, modem compression takes
care of it for people on slow links (I'm one of those). Compressing it just
makes it harder to comment on in a mailer.

This patch is good, but I still have some comments (nitpicks):


> diff -ubBr ximian-setup-tools/backends/filesys.pl.in xst-new/backends/filesys.pl.in
> --- ximian-setup-tools/backends/filesys.pl.in	Thu May 17 03:27:29 2001
> +++ xst-new/backends/filesys.pl.in	Sun May 20 15:52:03 2001
> @@ -655,6 +655,58 @@
>    return $table;
>  }
>  
> +# Get all instances from 'mount -p' output. Returns a filesys_table.
> +
> +sub xst_filesys_mount_cmd_parse

I just tried 'mount -p' with GNU mount, and it doesn't recognize the option.
So I propose naming this func xst_filesys_bsd_mount_parse().


> +{
> +  my ($table);
> +  my (@output);
> +
> +  $table = &xst_filesys_table_new ();
> +  @output = (readpipe ("mount -p"));

You should use something like

$mount_tool = &xst_file_locate_tool ("mount");
@output = (readpipe ($mount_tool . " -p"));

This should let the backend find the command even without a default path in
the environment. Also, the backend will log the search for the command (and
whether it was found or not) and cache its location. And we can put in a test
for ($mount_tool ne "") later if we need to break the function early.


> diff -ubBr ximian-setup-tools/backends/users-conf.in xst-new/backends/users-conf.in
> --- ximian-setup-tools/backends/users-conf.in	Sat May 19 02:22:04 2001
> +++ xst-new/backends/users-conf.in	Sun May 20 15:52:23 2001
> @@ -59,7 +59,7 @@
>  $name = "users";
>  $version = "0.1.0";
>  @platforms = ("redhat-5.2", "redhat-6.0", "redhat-6.1", "redhat-6.2", "redhat-7.0",
> -              "mandrake-7.2", "debian-2.2", "debian-woody", "suse-7.0");
> +              "mandrake-7.2", "debian-2.2", "debian-woody", "suse-7.0", "freebsd-4", "freebsd-5");

We should not add distros to these in CVS until we have tested both --get and
--set on a distro of that type. This reduces the risk that someone will check
it out (or that we do an accidental release of this code) and it breaks
someone's system without warning them.

Of course, we need to keep it for testing in our local versions, but we
shouldn't apply it to CVS until it works both ways.

We think can apply the configure.in changes right now (separately from the
rest), though.


--
Hans Petter

_______________________________________________
setup-tool-hackers maillist  -  setup-tool-hackers@ximian.com
http://lists.ximian.com/mailman/listinfo/setup-tool-hackers