prmjtime.cpp failed to compile on SunOS 5.*

RESOLVED FIXED

Status

()

RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: ginnchen+exoracle, Assigned: ginnchen+exoracle)

Tracking

({regression})

Trunk
All
OpenSolaris
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

10 years ago
See bug 411726 comment #40
(Assignee)

Comment 1

10 years ago
Created attachment 355922 [details] [diff] [review]
patch

copying "#if defined" from nsprpub/pr/src/misc/prtime.c
Assignee: general → ginn.chen
Attachment #355922 - Flags: review?(crowder)

Comment 2

10 years ago
What exactly are the build-errors we get here?  Why does this need to be based on OS defines?  Why can't we have nice things?

I'm cc:ing Jim and Ted for thoughts on the proper, configure-y way to do this.

Comment 3

10 years ago
OS/2 fails as well:
E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp: In function `size_t
   PRMJ_FormatTime(char*, int, const char*, PRMJTime*)':
E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:612: error: 'struct tm' has
   no member named 'tm_gmtoff'
E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:612: error: 'struct tm' has
   no member named 'tm_gmtoff'
E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:613: error: 'struct tm' has
   no member named 'tm_zone'
E:/usr/src/hg/mozilla-central/js/src/prmjtime.cpp:613: error: 'struct tm' has
   no member named 'tm_zone'

Comment 4

10 years ago
An AC_TRY_COMPILE test could decide whether those structure members were present.  Ginn, do you want to take a shot at it?  Assign the bug to me if you'd rather I did it.
(Assignee)

Comment 5

10 years ago
Created attachment 356466 [details] [diff] [review]
patch v2
Attachment #355922 - Attachment is obsolete: true
Attachment #356466 - Flags: review?(crowder)
Attachment #355922 - Flags: review?(crowder)

Comment 6

10 years ago
Comment on attachment 356466 [details] [diff] [review]
patch v2

Should this

> #ifdef HAVE_LOCALTIME_R
>     {
>         struct tm td;
>         time_t bogus = 0;
>         localtime_r(&bogus, &td);
>+#ifdef HAVE_TM_ZONE
>         a.tm_gmtoff = td.tm_gmtoff;
>         a.tm_zone = td.tm_zone;
>+#endif
>     }
> #endif

not better be

#if defined(HAVE_LOCALTIME_R) && defined(HAVE_TM_ZONE)
    {
        struct tm td;
        time_t bogus = 0;
        localtime_r(&bogus, &td);
        a.tm_gmtoff = td.tm_gmtoff;
        a.tm_zone = td.tm_zone;
    }
#endif

It seems to me that it doesn't really make sense to compile the first three lines but not the last two.

Comment 7

10 years ago
Comment on attachment 356466 [details] [diff] [review]
patch v2

Awesome, thanks.  Can you land, or does this need help?
Attachment #356466 - Flags: review?(crowder) → review+

Comment 8

10 years ago
Comment on attachment 356466 [details] [diff] [review]
patch v2

Actually, yes, I'm in agreement with comment #6.

Thanks.
Attachment #356466 - Flags: review+ → review-

Comment 9

10 years ago
(In reply to comment #5)
> Created an attachment (id=356466) [details]
> patch v2

Hi, Ginn --- the configure changes look good.

Mozilla can't use autoconf compile tests on Windows, because it doesn't know
how to invoke the Visual C++ compiler properly.  So all tests that do
compilation need to take place inside one of the 'if test -z
"$SKIP_COMPILER_CHECKS"' blocks that are already present in the configure
script.  In some patches I haven't landed yet, I've taken to hard-coding
answers for Windows in the block that goes like this:

if test -n "$_WIN32_MSVC"; then
    SKIP_PATH_CHECKS=1
    SKIP_COMPILER_CHECKS=1
    SKIP_LIBRARY_CHECKS=1
fi

... my thinking being that, since this is where we say that we can't run the
tests, it's the logical place to handle the alternative.  (I have some ideas on
how to support MSVC in configure, but I haven't tried them out yet.)

Some autoconf style points:

- Use AC_CACHE_CHECK --- you've basically written out its definition.
- Use 'yes' and 'no' for autoconf values, not 'true' and 'false'.
- It might be nice if the cache variable name included 'gmtoff'.
(Assignee)

Comment 10

10 years ago
Created attachment 356706 [details] [diff] [review]
patch v3

Thanks for the comments. I'm a newbie to autoconf.
Attachment #356466 - Attachment is obsolete: true
Attachment #356706 - Flags: review?(jim)
(Assignee)

Updated

10 years ago
Attachment #356706 - Flags: review?(crowder)

Updated

10 years ago
Attachment #356706 - Flags: review?(crowder) → review+

Updated

10 years ago
Attachment #356706 - Flags: review?(jim) → review-

Comment 11

10 years ago
Comment on attachment 356706 [details] [diff] [review]
patch v3

The autoconf code looks perfect now --- thanks!

However, since you use AC_TRY_COMPILE, this needs to be inside one of the SKIP_COMPILER_CHECKS blocks.  For example, up about 40 lines or so in js/src/configure.in from the place this patch inserts your code you'll see something that says:

fi # SKIP_COMPILER_CHECKS

that marks the end of a long block of checks that use AC_TRY_COMPILE, AC_TRY_RUN, and things like that.  Your block, comments and all, needs to inside that 'if', perhaps as the next sibling of the block that starts:

dnl ========================================================
dnl Autoconf test for gcc 2.7.2.x (and maybe others?) so that we don't
dnl provide non-const forms of the operator== for comparing nsCOMPtrs to
dnl raw pointers in nsCOMPtr.h.  (VC++ has the same bug.)
dnl ========================================================
(Assignee)

Comment 12

10 years ago
Created attachment 356904 [details] [diff] [review]
patch v4
Attachment #356706 - Attachment is obsolete: true
Attachment #356904 - Flags: review?(jim)

Comment 13

10 years ago
Comment on attachment 356904 [details] [diff] [review]
patch v4

Looks great!
Attachment #356904 - Flags: review?(jim) → review+
(Assignee)

Comment 14

10 years ago
Pushed

http://hg.mozilla.org/mozilla-central/rev/b2f2ce787cbe
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Blocks: 411726
No longer depends on: 411726
Keywords: regression

Updated

10 years ago
Flags: in-testsuite-
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.