Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2740353002:
AIX/SOLARIS/CYGWIN: Fix build failure due in PosixTimezoneCache (Closed)
DescriptionAIX/SOLARIS/CYGWIN: Fix build failure due in PosixTimezoneCache
R=littledan@chromium.org, ulan@chromium.org, bjaideep@ca.ibm.com
BUG=
Review-Url: https://codereview.chromium.org/2740353002
Cr-Commit-Position: refs/heads/master@{#43917}
Committed: https://chromium.googlesource.com/v8/v8/+/6fa1db1e2eaa645e5fa0b11b123c06894603bac9
Patch Set 1 #Patch Set 2 : Add fix for solaris and cygwin #Messages
Total messages: 17 (7 generated)
Sorry about this issue. Does the build need fixing for other platforms with custom methods here, like Solaris and Cygwin? Sorry, I don't have a testing setup for that. Maybe the right fix is put these methods in another file that's included in the build for just platforms that use the default method.
On 2017/03/11 21:12:32, Dan Ehrenberg wrote: > Sorry about this issue. Does the build need fixing for other platforms with > custom methods here, like Solaris and Cygwin? Sorry, I don't have a testing > setup for that. Maybe the right fix is put these methods in another file that's > included in the build for just platforms that use the default method. What should be the name for that new file then?
On 2017/03/13 16:57:38, Sampson wrote:
> On 2017/03/11 21:12:32, Dan Ehrenberg wrote:
> > Sorry about this issue. Does the build need fixing for other platforms with
> > custom methods here, like Solaris and Cygwin? Sorry, I don't have a testing
> > setup for that. Maybe the right fix is put these methods in another file
> that's
> > included in the build for just platforms that use the default method.
>
> What should be the name for that new file then?
How about making a subclass of PosixTimezoneCache which is called
PosixDefaultTimezoneCache which includes these two problematic methods, and
putting that in a new file src/base/platform/platform-posix-time.{cc,h}? They
can be left as pure virtual methods in platform-posix.cc. Or is this
overengineering? Do you have an opinion, Ulan?
On 2017/03/13 17:23:31, Dan Ehrenberg wrote:
> On 2017/03/13 16:57:38, Sampson wrote:
> > On 2017/03/11 21:12:32, Dan Ehrenberg wrote:
> > > Sorry about this issue. Does the build need fixing for other platforms
with
> > > custom methods here, like Solaris and Cygwin? Sorry, I don't have a
testing
> > > setup for that. Maybe the right fix is put these methods in another file
> > that's
> > > included in the build for just platforms that use the default method.
> >
> > What should be the name for that new file then?
>
> How about making a subclass of PosixTimezoneCache which is called
> PosixDefaultTimezoneCache which includes these two problematic methods, and
> putting that in a new file src/base/platform/platform-posix-time.{cc,h}? They
> can be left as pure virtual methods in platform-posix.cc. Or is this
> overengineering? Do you have an opinion, Ulan?
I would suggest using platform-posix-default.cc as the new file name. But I
feels like it is a little over-engineering after all. @littledan@chromium.org
@ulan@chromium.org
Sorry for the delay. lgtm, the approach seems fine to me. Please wait for Dan's approval too.
On 2017/03/17 10:48:28, ulan wrote: > Sorry for the delay. > > lgtm, the approach seems fine to me. > > Please wait for Dan's approval too. ulan, which approach seems fine--the one I am suggesting or the one Sampson implemented? If we go with the one in this patch, we'll need a follow-on patch for certain other platforms, I think.
Hi, For now can we land this CL, since its breaking build on aix and also nodejs using canary build in the community. Looks like the other 2 platform affected are Solaris and Cygwin, maybe we can extend the #if to include them? #if !V8_OS_AIX && !V8_OS_SOLARIS && !V8_OS_CYGWIN Thank you.
The CQ bit was checked by sampsong@ca.ibm.com
The patchset sent to the CQ was uploaded after l-g-t-m from ulan@chromium.org Link to the patchset: https://codereview.chromium.org/2740353002/#ps20001 (title: "Add fix for solaris and cygwin")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by sampsong@ca.ibm.com
Description was changed from ========== AIX: Fix build failure due in PosixTimezoneCache R=littledan@chromium.org, ulan@chromium.org, bjaideep@ca.ibm.com BUG= ========== to ========== AIX/SOLARIS/CYGWIN: Fix build failure due in PosixTimezoneCache R=littledan@chromium.org, ulan@chromium.org, bjaideep@ca.ibm.com BUG= ==========
The CQ bit was checked by sampsong@ca.ibm.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1489781080711380,
"parent_rev": "42487a841099d7fe72d32d0369718c8bcd8fc09f", "commit_rev":
"6fa1db1e2eaa645e5fa0b11b123c06894603bac9"}
Message was sent while issue was closed.
Description was changed from ========== AIX/SOLARIS/CYGWIN: Fix build failure due in PosixTimezoneCache R=littledan@chromium.org, ulan@chromium.org, bjaideep@ca.ibm.com BUG= ========== to ========== AIX/SOLARIS/CYGWIN: Fix build failure due in PosixTimezoneCache R=littledan@chromium.org, ulan@chromium.org, bjaideep@ca.ibm.com BUG= Review-Url: https://codereview.chromium.org/2740353002 Cr-Commit-Position: refs/heads/master@{#43917} Committed: https://chromium.googlesource.com/v8/v8/+/6fa1db1e2eaa645e5fa0b11b123c0689460... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/v8/v8/+/6fa1db1e2eaa645e5fa0b11b123c0689460... |
