143490 - checksetup.pl is calling functions not supported by windows

Status: RESOLVED FIXED

(Bugzilla :: Installation & Upgrading, defect)

Description

[Cédric Caron]

checksetup.pl is calling which and stty functions that ar not supported by 
windows.

Comment 1

[Cédric Caron]

Attachment: patch

this patch remove calls to functions not existing under windows

Comment 2

[Jouni Heikniemi]

Some evil tabs there in your patch, please use 4-space indentation per the style
guide.

Also: 
>$mysql_binaries = "c:\\\\mysql\\\\bin"

This doesn't make much sense to me. Why double backslashes? It would be quite a
bit cleaner to say $mysql_binaries = 'c:\mysql\bin' or if you insist on the
double quotes, "c:\\mysql\\bin" then.

Reassigning to patch author and proposing 2.18 per roadmap's win32 statement.

Comment 3

[Cédric Caron]

the reason for this magin line : $mysql_binaries = "c:\\\\mysql\\\\bin"

to store a c:\mysql\bin in the $mysqlpath variable I need to write 
$mysqlpath = "c:\\mysql\\bin";
in the localconfig file

and to store "c:\\mysql\\bin" in $mysql_binaries I need to write
$mysql_binaries = "c:\\\\mysql\\\\bin"
in the perl source code

Comment 4

[Cédric Caron]

Attachment: patch v2

updated patch with tab removed

Comment 5

[Jouni Heikniemi]

Comment on attachment 83328 [details] [diff] [review]
patch v2

>Index: checksetup.pl
>===================================================================

>+# which dosn't exist under window just provide a reasonable default

Nit: fix the typos: dosn't -> doesn't, "window" -> "Windows, so"

>+    $mysql_binaries = "c:\\\\mysql\\\\bin";

Ok, your explanation cleared this out. It's still looks horrible 
and the worst part is that the sysadmin configuring the installation
has to maintain the double-backslashes to avoid really obscure
errors. Would it be possible to write this parameter out in single
quotes to localconfig? This way Windows configuration would be less 
error prone and easier to administer.

Escaping the strings this early doesn't sound logical to me either; 
I think the values should be entered here as is (DOS paths with 
single quotes to preserve readability, if possible) and if escaping 
is necessary, it should be done when the values are actually
written out.

Comment 6

[Cédric Caron]

Attachment: patch v3

using single quote seems to work

Comment 7

[Jouni Heikniemi]

Well, it worked on my Win32 installation. I wonder if the change of quoting
character affects Unix platforms adversely? Wouldn't think so, but reviewers
hopefully will try that out, my unix installation is thoroughly wasted right now. 

Adding patch and review keywords, since those thingies tend to attract higher
attention ;-)

Comment 8

[Bradley Baetz (:bbaetz)]

Is there a standard perl module to do the echo-disabling stuff? Preferably one
which works on windows.

It should be a standard one, though - thers no point in requireing an extra perl
module just for checksetup.

Comment 9

[Cédric Caron]

I tryed to disable echo under Windows without any success!! ANSI sequances are 
not supported in the win32 console and I didn't find any perl module to do the 
job...

Comment 10

[Bradley Baetz (:bbaetz)]

Comment on attachment 83330 [details] [diff] [review]
patch v3

Term::ReadKey may work, but the perl faqs do say to use stty.

Changing localconfig vars won't affect existing files, bt thats OK.

Why do you need this, though? If 'stty' doesn't exist, then surely the system
call will just fail, and nothing will get done - we never check the return
code.

Comment 11

[Jouni Heikniemi]

We should avoid running operating system commands unless we're certain of what
they are doing. If a future version of Windows contains a stty.exe which does
something totally different from /bin/stty, we might get into trouble. This can
of course happen already now, if someone has installed a custom piece of
software named "stty". In the unix world this would be fairly unlikely, but not
at all impossible on Windows systems.

So yes, I think adding operating system checks for stty calls is a good idea. If
a common perl module would be found, that would of course be still much better.

Comment 12

[Cédric Caron]

If you run the current version of checksetup under windows you receive an error 
message for each call of stty and which...

Comment 13

[Bradley Baetz (:bbaetz)]

According to npm.webtools, the call to getgrname fails, too.

We either need to support a separate webservergroup on windows, or (more
likely), error out if we have a webservergroup and are on windows.

I suspect that the 2nd option is the only one we can do on win9x, but I don't
know about ntfs systems.

Comment 14

[Cédric Caron]

checksetup already contains this comment

# Theres no webservergroup, this is very very very very bad.
# However, if we're being run on windows, then this option doesn't
# really make sense. Doesn't make it any more secure either, though,
# but don't print the message, since they can't do anything about it.

under Windows the best solution is probabyl to:

1. set the default value of webservergroup to ""
2. generate a warning if webservergroup not set to ""
3. meabe ignore webservergroup  from the localconfig and set it to ""

Comment 15

[Bradley Baetz (:bbaetz)]

I prefer 1, and 2, except an error instead of a warning.

Maybe only declare the LocalVar if we're not on win32? That would fix new
installs, I guess. We'd still have to support existing ones with that option set.

Comment 16

[Jouni Heikniemi]

I don't see why we should generate an error. It would only create unnecessary
incompatibility between setups on Unix and Windows. A warning should be sufficient. 

I also think that any webservergroup settings should be ignored on Win32, as
using NTFS for this probably isn't worth the problems involved (see bug 140355
comment 9). So to sum it up, I'd go for Cedric's points 1 and 2, with an added
documentation note (see the bug previously mentioned for a proposal). 

The huge unix-technical comments above localconfig's $webservergroup line should
also be suppressed on Win32, since the text is quite confusing for windows admins. 

Comment 17

[Cédric Caron]

the point 2 need to be an error and stop checksetup.

curently if you don't est $webservergroup to "" under windows checksetup stop 
with an error when calling getgrnam

Comment 18

[Cédric Caron]

My english is not very good can you provide me the error message to display 
when $webservergroup is not set to "" under windows

Thanks

Cedric

Comment 19

[Jouni Heikniemi]

Umh, wouldn't it be easier just to skip the block with all the group/chmod stuff
(including the getgrnam) if we're on Win32? That block doesn't contain anything
else but chmod calls, and those don't generally make much sense on Windows
anyway. Then we could have this as a warning.

As for the message, I was thinking something on the lines of

"Warning: You have set webservergroup in your localconfig. Please understand that
this does not bring you any security when running under Windows. Verify that the
file permissions in your Bugzilla directory are suitable for your system. Avoid
unnecessary write access."

Comment 20

[Cédric Caron]

by forcing webservergroup to be "" you don't have to do any aditional check 
enywhere in the code.

If a different value is allowed we may have to add a win32 check to all 
webservergroup tet in all present if future code

Comment 21

[Jouni Heikniemi]

>by forcing webservergroup to be "" you don't have to do any aditional check 
>enywhere in the code.

Maybe, but the result won't be much cooler there. If you take a look at the if
($my_webservergroup) block near lines 1020-1070 (current trunk tip), it will use
$( and $< if no group has been specified. This is not very cool either (on
Win32). I don't have a Win32 perl installation handy right now, but I have some
doubts about $( and $< working equally on both Windows and Unix. 

So the whole if block (including the else section) should imo be ignored on
Win32 anyway, even if you force the $my_webservergroup to be "".

Comment 22

[Cédric Caron]

On my PC logged with administrator rights $< and $( return 0

Comment 23

[Jouni Heikniemi]

Ditto, also for $> and $). W2k, AS Perl 5.6.1. 

Using chown with zero group and user ids sounds stupid. I still like the idea of
iffing out the whole permission-setting thing when $^O eq 'Win32'. 

Comment 24

[Cédric Caron]

Attachment: patch v4

this version set the default value of webservergroup to "", print a warning if
webservergroup is not = "" and skip the group/chmod code

I everybody is happy with the warning i it's ok for me but I prefere the error
version...

Comment 25

[Cédric Caron]

is the comment "Reminder: Bugzilla now requires version 8.7 or later of 
sendmail."  printed by checksetup realy needed under Windows ???

Comment 26

[Jouni Heikniemi]

In most cases probably no, since I don't think many people use Windows sendmail
implementations anyway (and wonder if they would even be sufficiently compatible?). 

Comment 27

[Jouni Heikniemi]

Comment on attachment 84086 [details] [diff] [review]
patch v4

Bitrotten. Surprisingly many hunks patched cleanly, though...

Comment 28

[Marc Bauer]

if this is perl and you use $mysql_binaries = "c:\\mysql\\bin"; as path this 
is not a good idea! use $mysql_binaries = "c:/mysql/bin"; instead. this 
forward slashes are full supported on win32!

Comment 29

[Marc Bauer]

additional to this a $mysql_binaries = "c:\mysql\bin"; will not realy work for 
my knowledge... perl needs minimum a "\\" for result in a "\". therefor 
use "/" instead - on win32 too!

Comment 30

[Zach Lipton [:zach]]

Comment on attachment 84086 [details] [diff] [review]
patch v4

bitrotten

Comment 31

[Jason Pyeron]

I am investigating the webservergroup issue, next week.
see bug 208521.

cygwin on win2k/ntfs does support chmod/uid/gid/etc...

the names are very convoluded.

I have issues all the time when using tar/scp.

I just have to look at how cygwin perl 5.8.x handles it.

I will not be trying ActiveState's perl, as they do not handle CPAN shell stuff.

Comment 32

[Jean-Sebastien Guay]

Why are you looking at this from a Cygwin perspective? ActiveState is the 
standard Perl on Windows. Requiring users to install a Cygwin environment for 
Bugzilla is overkill. Plus, Cygwin comes with its own version of Apache, which 
is also not the standard Apache on Windows. So it introduces another variable.

I really think this should work on ActiveState by default.

Comment 33

[Jason Pyeron]

We use cygwin only since it is corporate policy and our federal customers 
(including the DoD) have approved it. 

Beyond that, it increases portability between systems. We can take an arbitrary 
linux source rpm, and install it on windows.

It further supports "security" of the filesystem, logon, etc. 

In ActiveState these act differently:

Perl functions affected by this are:

"-X", "binmode", "chmod", "chown", "chroot", "crypt", "dbmclose",
"dbmopen", "dump", "endgrent", "endhostent", "endnetent", "endpro-
toent", "endpwent", "endservent", "exec", "fcntl", "flock", "fork",
"getgrent", "getgrgid", "gethostent", "getlogin", "getnetbyaddr", "get-
netbyname", "getnetent", "getppid", "getprgp", "getpriority", "getpro-
tobynumber", "getprotoent", "getpwent", "getpwnam", "getpwuid", "get-
servbyport", "getservent", "getsockopt", "glob", "ioctl", "kill",
"link", "lstat", "msgctl", "msgget", "msgrcv", "msgsnd", "open",
"pipe", "readlink", "rename", "select", "semctl", "semget", "semop",
"setgrent", "sethostent", "setnetent", "setpgrp", "setpriority", "set-
protoent", "setpwent", "setservent", "setsockopt", "shmctl", "shmget",
"shmread", "shmwrite", "socket", "socketpair", "stat", "symlink",
"syscall", "sysopen", "system", "times", "truncate", "umask", "unlink",
"utime", "wait", "waitpid"

Comment 34

[Jason Pyeron]

This works for me
Microsoft Windows 2000 [Version 5.00.2195]
Bugzilla 2.17.4
Perl v5.8.0 built for cygwin-multi-64int
webservergroup=""

 see bug 208521

Comment 35

[Jean-Sebastien Guay]

Re : Comment #33

I didn't say that you can't use Cygwin. I agree, it makes most of the "needed-
for-Win32BZ" bugs go away. What I said is that it is not the standard 
environment for using Bugzilla on Windows. For the general user, it is hard to 
install, bulky, and has lots of implications on the general use of the system 
from the command line. Requiring that users install Cygwin to run Bugzilla 
would be overkill.

So yes, I agree that most of these bugs do not present themselves to you. If 
you want to help fix them, though, it's not very useful to go to each Win32 bug 
saying that in your environment, they are non-existent. They still need to be 
fixed in the standard Bugzilla environment, which is ActiveState Perl + Apache 
or IIS.

Comment 36

[Jason Pyeron]

re Comment 35

For the general user, it [cygwin] is hard to install, bulky, and has lots of 
implications on the general use of the system from the command line. 

I have to disagree with this statement, installing it does not change how you 
use your CMD line. 

Hard to install? (1) http://cygwin.com/setup.exe (2) install from internet, 
select mirror, defaults, etc. (3) select full install (4) no icons / start menu 
(5) [HARD PART] System:Environment Vars:PATH, add c:\cygwin\bin

I do agree it is bulky, space hog (unless you spend time triming down the 
install) but it work wonderfully.

Comment 37

[Jason Pyeron]

re comment 35:

> If you want to help fix them, though, it's not very useful to go to each 
Win32 bug saying that in your environment, they are non-existent. 

This shows those who need an INSTANT/now fix that it can be done. What these 
bugs are are win32-activestate bugs.

As I have shown it can run on win32 (win2k) ... IIS 5.0

I will be testing Apache next month. All our motives are client driven, I will 
post the results so others can benifit from them. Even if it shows a slightly 
different solution from what was expected. (ie bug 188570)

As far as ActiveState Perl testing? I dont expect us to ever be able to throw 
our resources to it, unless a client requests(pays for) it.

Comment 38

[Jason Pyeron]

re comment 35:

> If you want to help fix them, though, it's not very useful to go to each 
Win32 bug saying that in your environment, they are non-existent. 

This shows those who need an INSTANT/now fix that it can be done. What these 
bugs are are win32-activestate bugs.

As I have shown it can run on win32 (win2k) ... IIS 5.0

I will be testing Apache next month. All our motives are client driven, I will 
post the results so others can benifit from them. Even if it shows a slightly 
different solution from what was expected. (ie bug 188570)

As far as ActiveState Perl testing? I dont expect us to ever be able to throw 
our resources to it, unless a client requests(pays for) it.

Comment 39

[Jean-Sebastien Guay]

I have no interest in arguing with you. The standard environment for Bugzilla 
is ActiveState Perl, without Cygwin. The bugs should be fixed in that 
environment.

I, for one, will not install 80-300MB of stuff just to fix 5 bugs that should 
be fixed anyways. And I don't believe anyone else that wants to use Bugzilla on 
Windows will either.

Comment 40

[Jouni Heikniemi]

With some luck, bug 224698 and bug 224476 will reduce the scope of this bug
somewhat. Anyway, unless the original patch author or somebody else picks up the
ball (or expresses the desire to do so) soon, I'll take this when I'm done with
some of the other Win32 bugs.

Comment 41

[vignesh]

Attachment: checksetup.pl is calling functions not suported by windows

please check it and tell me how to install bugzilla for the win32. i cannot
change my operating system to linux.

Comment 42

[Jouni Heikniemi]

Comment on attachment 137777 [details]
checksetup.pl is calling functions not suported by windows

This is not a support board. Please take a look at the newsgroup
netscape.public.mozilla.webtools archives for some installation tips. And read
the documentation.

Comment 43

[Andrei Benea]

Attachment: v1

Comment 44

[Andrei Benea]

Attachment: v2

Fixed an indentation issue.

Comment 45

[Vlad Dascalu]

Comment on attachment 143099 [details] [diff] [review]
v2

+	 # provide a reasonable default for Windows
+	 $mysql_binaries = 'c:/mysql/bin';

This doesn't work on older versions of Windows. You need \ instead of / in
order to navigate the subdirectories.

+    my $cvs_executable = '';
+    if ($^O !~ /MSWin32/i) {
+	 $cvs_executable = `which cvs`;

Nit: In this case $cvs_executable gets overwritten if we are running on Unix.
Would it make sense to set it to "" only in the "else" branch?

Comment 46

[Andrei Benea]

Attachment: v3

Comment 47

[Vlad Dascalu]

Comment on attachment 143204 [details] [diff] [review]
v3

r=vlad, assuming you tested it.

Comment 48

[Dave Miller [:justdave]]

(In reply to comment #45)
> +	 $mysql_binaries = 'c:/mysql/bin';
> 
> This doesn't work on older versions of Windows. You need \ instead of / in
> order to navigate the subdirectories.

I'm pretty sure it does work.  ActiveState translates it.

Comment 49

[Dave Miller [:justdave]]

Comment on attachment 143204 [details] [diff] [review]
v3

I think in a couple of these cases, it might be easier to read if you swapped
the if condition and did if Win32 then foo else the normal stuff, but this
works, and it's not exactly unreadable :)

Comment 50

[Dave Miller [:justdave]]

This will require changes to the os-specific installation documentation (one
less step to perform ;)  You can + the documentation flag when the doc change is
checked in.

Comment 51

[Cédric Caron]

Hello,

Good to see that somone finished the job ;-)

An other win32 "incompatibility" is the message describing how to obtain the 
perl modules.

Active State users need to know how to install the modules using PPM

Comment 52

[Vlad Dascalu]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.265; previous revision: 1.264
done

Comment 53

[Andrei Benea]

Attachment: install

"2.4.1.3.1. Changes to checksetup.pl" section removed.

Comment 54

[Andrei Benea]

Attachment: install v2

Comment 55

[Vlad Dascalu]

Comment on attachment 143272 [details] [diff] [review]
install v2

After talking with andrei and kiko and noticing that we are left with:

2.4.1.3. Code changes required to run on win32
2.4.1.3.1. Changes to BugMail.pm

and no other item (like a 2.4.1.3.2), Andrei said he'll rewrite the patch in
order not to have a single item in the 2.4.1.3 node.

Comment 56

[Andrei Benea]

Attachment: install v3

Comment 57

[Christian Reis]

Comment on attachment 143282 [details] [diff] [review]
install v3

>Index: docs/xml/installation.xml
>-      <section id="win32-code-changes">
>-        <title>Code changes required to run on win32</title>
>+      <section id="win32-code-bugmail">
>+        <title>Changes to <filename>BugMail.pm</filename></title>

This title makes little sense in the context; it should mention win32. I think
the correct approach is to leave the title as-is, and ->

>+        <para>To make bug email work on Win32 (until

Change this to say:

  Bugzilla on win32 is mostly supported out of the box; one remaining issue is
related to bug email. To make bug email...

>+        <ulink url="http://bugzilla.mozilla.org/show_bug.cgi?id=84876">bug
>+        84876</ulink> lands), the
>+        simplest way is to have the Net::SMTP Perl module installed and 
>+        change this:</para>

and here say "change this line in the file Bugzilla/Bugmail.pm":

Try reading the section before this one and after this one to make sure it
still makes sense in the context to ensure my suggestions make sense :-)

Comment 58

[Dave Miller [:justdave]]

Marking fixed since the code patch is checked in.  The documentation part is
still considered open while the documentation flag is '?'.

Comment 59

[:glob ✱]

*** Bug 230574 has been marked as a duplicate of this bug. ***

Comment 60

[GavinS]

Attachment: docs changes v4

I think I've made the changes kiko suggested to the docs patch...

Comment 61

[Vlad Dascalu]

Comment on attachment 163258 [details] [diff] [review]
docs changes v4

Hmm, we could mention the sendmail Windows wrapper that glob made -
http://bugzilla.glob.com.au/guides/windows/

(there are pretty cool toys there that could be mentioned as well). Anyway, I'd
open a new bug about that and leave that in another patch. (Gavin - any time to
search for such a bug and open one if it's not already opened? Thanks! :) )

Comment 62

[Vlad Dascalu]

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <-- 
installation.xml
new revision: 1.77; previous revision: 1.76
done

Checking in docs/xml/installation.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/installation.xml,v  <-- 
installation.xml
new revision: 1.72.2.5; previous revision: 1.72.2.4
done

Comment 63

[GavinS]

(In reply to comment #61)
> (there are pretty cool toys there that could be mentioned as well). Anyway, I'd
> open a new bug about that and leave that in another patch. (Gavin - any time to
> search for such a bug and open one if it's not already opened? Thanks! :) )
> 

Created bug#266004 for this

Comment 64

[Vlad Dascalu]

*** Bug 273345 has been marked as a duplicate of this bug. ***