Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rpn2 test fails on non-glibc Linux #1012

Closed
awilfox opened this issue Jun 4, 2019 · 6 comments · Fixed by #1017
Closed

rpn2 test fails on non-glibc Linux #1012

awilfox opened this issue Jun 4, 2019 · 6 comments · Fixed by #1017

Comments

@awilfox
Copy link

awilfox commented Jun 4, 2019

Describe the bug
musl libc does not provide _NL_TIME_FIRST_WEEKDAY. This causes find_first_weekday to return 1 (Monday), and the rpn2 test requires the first weekday to be 0 (Sunday).

To Reproduce
Steps to reproduce the behavior:

  1. Install a Linux distribution using musl libc.
  2. Unpack rrdtool-1.7.2.
  3. Build.
  4. Run make check.

Expected behavior
Passing tests (which do occur if either rpn2 test is disabled, or hardcoded return value from find_first_weekday is 0 instead of 1).

Screenshots
Test failure

Desktop (please complete the following information):

  • OS: Adélie Linux 1.0-BETA3 on 64-bit PowerPC
  • Browser: Firefox
  • Version: 1.7.2, release tarball

Additional context
This is similar to #661 but it isn't the same thing, because _NL_TIME_FIRST_WEEKDAY really doesn't exist on musl libc.

@nirgal
Copy link
Contributor

nirgal commented Jun 4, 2019

Out of curiosity, what does locale -ck LC_TIME yields? Here:

$ locale -ck LC_TIME
LC_TIME
abday="Sun;Mon;Tue;Wed;Thu;Fri;Sat"
day="Sunday;Monday;Tuesday;Wednesday;Thursday;Friday;Saturday"
abmon="Jan;Feb;Mar;Apr;May;Jun;Jul;Aug;Sep;Oct;Nov;Dec"
mon="January;February;March;April;May;June;July;August;September;October;November;December"
am_pm="am;pm"
d_t_fmt="%a %d %b %Y %T %Z"
d_fmt="%d/%m/%y"
t_fmt="%T"
t_fmt_ampm="%l:%M:%S %P %Z"
era=
era_year=""
era_d_fmt=""
alt_digits=
era_d_t_fmt=""
era_t_fmt=""
time-era-num-entries=0
time-era-entries="S"
week-ndays=7
week-1stday=19971130
week-1stweek=4
first_weekday=2
first_workday=2
cal_direction=1
timezone=""
date_fmt="%a %e %b %H:%M:%S %Z %Y"
time-codeset="UTF-8"

@awilfox
Copy link
Author

awilfox commented Jun 7, 2019

musl doesn't have a locale command, unfortunately.

@nirgal
Copy link
Contributor

nirgal commented Jun 10, 2019

Ouch, obviously this is an issue for weekly data, as its depends of the first day of the week (Monday or Sunday depending of local folklore).

We are assuming this is Monday when HAVE__NL_TIME_WEEK_1STDAY is undefined.

I can see rpn2 xport test runs with "LC_TIME=C" meaning that we expect the test to run with the first day of the week being Sunday.

Two options:

  • Change the rpn2 test to run with a LC_TIME where weeks starts on Monday.
  • Change the default values of firstday of the week from Monday to Sunday if HAVE__NL_TIME_WEEK_1STDAY is undefined (3 occurrences in the source).

Tobias: Any suggestion?

@oetiker
Copy link
Owner

oetiker commented Jun 11, 2019

I guess assuming sunday for the weekstart when HAVE__NL_TIME_WEEK_1STDAY is not present is the 'right thing' thing in the sense of least surprise ... after all this is all US centric and in the US sunday is the start of the week, so if the system has not bothered to support localization at that level we should keep it US-style.

@nirgal
Copy link
Contributor

nirgal commented Jun 13, 2019

Then this should do the trick:

diff --git a/src/rrd_graph.c b/src/rrd_graph.c
index f5d2cdec..62c3e645 100644
--- a/src/rrd_graph.c
+++ b/src/rrd_graph.c
@@ -1568,7 +1568,7 @@ static int find_first_weekday(
         }
         first_weekday = (week_1stday + first_weekday - 1) % 7;
 #else
-        first_weekday = 1;
+        first_weekday = 0;
 #endif
     }
     return first_weekday;
diff --git a/src/rrd_rpncalc.c b/src/rrd_rpncalc.c
index 0f54c6be..84f69211 100644
--- a/src/rrd_rpncalc.c
+++ b/src/rrd_rpncalc.c
@@ -564,7 +564,7 @@ static int find_first_weekday(void){
         }
         first_weekday=(week_1stday + first_weekday - 1) % 7;
 #else
-        first_weekday = 1;
+        first_weekday = 0;
 #endif
     }
     return first_weekday;

@oetiker
Copy link
Owner

oetiker commented Jun 13, 2019

indeed :) can you make a PR ?

c72578 pushed a commit to c72578/rrdtool-1.x that referenced this issue Jun 16, 2019
- Set first_weekday_to 0 (Sunday), when HAVE__NL_TIME_WEEK_1STDAY
  is not defined
- Fixes: oetiker#1012
c72578 pushed a commit to c72578/rrdtool-1.x that referenced this issue Jun 16, 2019
- Set first_weekday to 0 (Sunday), when HAVE__NL_TIME_WEEK_1STDAY
  is not defined
- Fixes: oetiker#1012
oetiker pushed a commit that referenced this issue Jun 16, 2019
- Set first_weekday to 0 (Sunday), when HAVE__NL_TIME_WEEK_1STDAY
  is not defined
- Fixes: #1012
chrisburr added a commit to chrisburr/staged-recipes that referenced this issue Aug 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants