Opened 8 years ago

Closed 8 years ago

Last modified 6 years ago

#13131 closed patch (fixed)

purple_str_to_time() fails test during make check

Reported by: fangism Owned by: datallah
Milestone: 2.7.10 Component: libpurple
Version: 2.7.8 Keywords:
Cc:

Description (last modified by rekkanoryo)

Didn't see a trac ticket for this yet, don't want to lose track of it. Recently added test failing since 2.7.8.

make  check-TESTS
Running suite(s): Master Suite
 Cipher Suite
 Jabber Caps Functions
 Jabber Utility Functions
 Jabber SASL SCRAM functions
 QQ
 Yahoo Utility Functions
 Utility Functions
98%: Checks: 86, Failures: 1, Errors: 0
../../../libpurple/tests/test_util.c:160:F:Time:test_util_str_to_time:0: Assertion '1282941722 == purple_str_to_time("2010-08-27.134202-0700PDT", FALSE, NULL, NULL, NULL)' failed
FAIL: check_libpurple
================================
1 of 1 test failed
Please report to devel@pidgin.im
================================

My platform is Mac OS X 10.4/10.6, but others have reported it on devel mailing list as well.

Attachments (3)

configure.ac.diff (1.2 KB) - added by NaderM 8 years ago.
util.c.2.diff (7.4 KB) - added by NaderM 8 years ago.
util.c.diff (7.4 KB) - added by NaderM 8 years ago.

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by NaderM

I went through purple_str_to_time and rewrote most of it. I've added the patch as an attachment

The helper function, mktime_utc, was taken from glib, I'm not sure if that would be considered a problem.

comment:2 Changed 8 years ago by rekkanoryo

  • Milestone set to Patches Needing Review
  • Owner set to datallah
  • Type changed from defect to patch

comment:3 Changed 8 years ago by darkrain42

NaderM, what is the license of the file in glib that you included that from?

comment:4 Changed 8 years ago by NaderM

/* GLIB - Library of useful routines for C programming
 * Copyright (C) 1995-1997  Peter Mattis, Spencer Kimball and Josh MacDonald
 *
 * This library is free software; you can redistribute it and/or
 * modify it under the terms of the GNU Lesser General Public
 * License as published by the Free Software Foundation; either
 * version 2 of the License, or (at your option) any later version.
 *
 * This library is distributed in the hope that it will be useful,
 * but WITHOUT ANY WARRANTY; without even the implied warranty of
 * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
 * Lesser General Public License for more details.
 *
 * You should have received a copy of the GNU Lesser General Public
 * License along with this library; if not, write to the
 * Free Software Foundation, Inc., 59 Temple Place - Suite 330,
 * Boston, MA 02111-1307, USA.
 */

In short, GPLv2

comment:5 Changed 8 years ago by rekkanoryo

  • Description modified (diff)

More precisely, LGPLv2.

comment:6 Changed 8 years ago by darkrain42

More more precisely, LGPLv2 or /later/.

If this patch is accepted, we should note that somehow.

comment:7 Changed 8 years ago by datallah

Do we really need to note that?

Perhaps I'm having a lapse of license consciousness here, but I believe that we can legally take "LGPLv2 or later" code and effectively make that GPLv2 right?

Just so nobody gets the wrong idea, I'm not at all trying to argue against attributing the origin of code at all!

comment:8 Changed 8 years ago by NaderM

I asked on #gtk and the response was that if our stuff is license compatible (LGPLv2.1+ or GPLv2+) then we should be fine, as long as we give credit. (Though I would update the credit to: "glib/gtimer.c", rather than "glib")

libpurple/util.c is GPLv2 though, so the two would be license compatible in this case

comment:9 Changed 8 years ago by NaderM

I fixed the attribution comment on mktime_utc, pretty sure we're all good on that front then

comment:10 Changed 8 years ago by rekkanoryo

We should probably do something like "Authors of Glib's glib/gtimer.c" in the COPYRIGHT file since whoever wrote that function holds copyright on that code. This isn't something that needs an updated patch, it just needs to be done right around when the patch is committed.

comment:11 Changed 8 years ago by rekkanoryo

Has anyone actually commented on whether this patch is acceptable as-is? I'm not confident enough to just commit it without guidance.

Changed 8 years ago by NaderM

Changed 8 years ago by NaderM

Changed 8 years ago by NaderM

comment:12 Changed 8 years ago by NaderM

I updated the patch to make mktime_utc use gmtime when available and fall back on the custom code, as glib does (Had it omitted originally because I hadn't updated configure.ac to check for it)

comment:13 Changed 8 years ago by rekkanoryo

  • Milestone changed from Patches Needing Review to 2.7.10

rlaager seems to have a couple concerns that he mentioned in #pidgin:

10:16 < rlaager> thundpressor: Also, I haven't reviewed the patch
                 in 13131 yet, but why are you importing code from
                 glib? They must not expose this as a public function?

comment:14 Changed 8 years ago by NaderM

Yeah, it's a local helper function to g_time_val_from_iso8601(). That function is pretty similiar to purple_str_to_time(), but unfortunately we cannot use the it because it doesn't support setting the out variables (tm/tz_off/rest)

comment:15 Changed 8 years ago by morshed.nader@…

  • Resolution set to fixed
  • Status changed from new to closed

(In 3435ea1c22455d6bafdda7845e20a498b39f839b):
Fix purple_str_to_time(). Fixes #13131.

comment:16 Changed 8 years ago by rekkanoryo@…

comment:17 Changed 8 years ago by rekkanoryo

This passed make check and after running with it for about 18 hours I couldn't see anything in logging that broke, so it seemed good enough to me. (How this could have possibly affected logging is really beyond me.)

comment:18 Changed 8 years ago by rekkanoryo@…

(In 342a95ba56375edcb99f196a32acfdb530660c2a):
Qulogic and darkrain point out that this bit shouldn't be here. Refs #13131.

comment:19 Changed 6 years ago by Richard Laager <rlaager@…>

(In [f93f8b0f36c3]):
Fix purple_str_to_time()'s tz_off sign handling

This was introduced with the near-rewrite of this function in the patch from #13131 (hg commit 6a74f42c8c04). It presumably went unnoticed this long because most (all default?) locales don't use the timezone offset when outputting dates.

Refs #13131 Fixes #15675

Note: See TracTickets for help on using tickets.
All information, including names and email addresses, entered onto this website or sent to mailing lists affiliated with this website will be public. Do not post confidential information, especially passwords!