224588 - Unify ($^O =~ /MSWin/) checks

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Jouni Heikniemi]

We have some OS checks around the code, and before all the Win32bz bugs are
resolved, there is probably going to have to be some more. It would be a good
idea to unify "are we running on Win32"-checks into a common sub somewhere,
maybe globals.pl.

Comment 1

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 2

[Jouni Heikniemi]

Attachment: v1

Comment 3

[Jouni Heikniemi]

Attachment: v1 (for real!)

The last attachment was a fumble.

Comment 4

[Jouni Heikniemi]

Attachment: v1 (really for real...)

I'm a bit confused, but this should really be it...

Comment 5

[:glob ✱]

t/008filter.t has two windows tests.

Comment 6

[Jouni Heikniemi]

Attachment: v2

Good catch.

Comment 7

[:glob ✱]

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

looks good to me.

Comment 8

[Dave Miller [:justdave]]

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

as much as I like the concept here, something about this patch is making me
nervous...  and I'm not sure what.  Something's tingling in the back of my head
and I can't quite put my finger on it...  If bbaetz could find time to take a
quick look at it, I'd really appreciate it.

Comment 9

[Dave Miller [:justdave]]

OK, I think I figured out what was making me go "hmmmm" here....

I'm not sure we should be pulling in Bugzilla::Util from inside the tests.  That
may be a good place to leave the raw $^O check.

I'll approve patch "v1 (really for real...)" right now...  if you want v2 in,
I'll still wait for bbaetz' opinion.

Comment 10

[Jouni Heikniemi]

I had the same doubt, but when I noted 009bugwords is using Bugzilla::Util
anyway, decided to try it. Since it seems to work quite fine, I don't see the
problem (apart from edge cases like Bugzilla::Util not compiling). I'm fine with
waiting for bbaetz though, but we should note that the issue isn't limited to
this patch.

I'll wait for further comments from either of you before checking anything in.

Comment 11

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 12

[Max Kanat-Alexander]

Bugzilla::Util can't be brought in before the BEGIN checks happen.
Bugzilla::Util uses Date::Format, which we check for the existence of. I'm not
entirely sure the technical details of BEGIN, but if Bugzilla::Util is actually
used before the BEGIN block, then attachment 153638 [details] [diff] [review] will prevent a valid
Date::Format check.

Comment 13

[Max Kanat-Alexander]

Why don't you just make it a constant, in Bugzilla::Constants? It can be WIN_32
or IS_WINDOWS or something like that.

Comment 14

[Max Kanat-Alexander]

Attachment: v3

Okay, here we go. This reminds me that we also need to clean up the tests (to work with "prove" instead of "runtests.pl") and probably also the top of Bugzilla::CGI could use some cleanup. (But that's for another bug).

Comment 15

[Frédéric Buclin]

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

>Index: testserver.pl

>-if ($^O !~ /MSWin32/i) {
>+if (ON_WINDOWS) {

You reversed the logic. It's !~ ,  not =~ .


>-elsif ($^O !~ /MSWin32/i) {
>+elsif (ON_WINDOWS) {

Same here.


>-    if ($^O !~ /MSWin32/i) {
>+    if (ON_WINDOWS) {

And here.

Comment 16

[Max Kanat-Alexander]

Attachment: v4

Comment 17

[Frédéric Buclin]

Comment on attachment 395947 [details] [diff] [review]
v4

Looks good. r=LpSolit

Comment 18

[Max Kanat-Alexander]

Checking in showdependencygraph.cgi;
/cvsroot/mozilla/webtools/bugzilla/showdependencygraph.cgi,v  <--  showdependencygraph.cgi
new revision: 1.69; previous revision: 1.68
done
Checking in testserver.pl;
/cvsroot/mozilla/webtools/bugzilla/testserver.pl,v  <--  testserver.pl
new revision: 1.22; previous revision: 1.21
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.48; previous revision: 1.47
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.115; previous revision: 1.114
done
Checking in Bugzilla/Config/Common.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Config/Common.pm,v  <--  Common.pm
new revision: 1.27; previous revision: 1.26
done
Checking in t/001compile.t;
/cvsroot/mozilla/webtools/bugzilla/t/001compile.t,v  <--  001compile.t
new revision: 1.18; previous revision: 1.17
done
Checking in t/008filter.t;
/cvsroot/mozilla/webtools/bugzilla/t/008filter.t,v  <--  008filter.t
new revision: 1.31; previous revision: 1.30
done
Checking in t/012throwables.t;
/cvsroot/mozilla/webtools/bugzilla/t/012throwables.t,v  <--  012throwables.t
new revision: 1.6; previous revision: 1.5
done