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

test1148: disable if decimal separator is not a point #2786

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@MarcelRaad
Member

MarcelRaad commented Jul 24, 2018

LC_NUMERIC doesn't work on Windows, so just replace the decimal comma
with a decimal point in the expected result.

@MarcelRaad MarcelRaad added the tests label Jul 25, 2018

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jul 25, 2018

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 25, 2018

There are other decimal separators, but I couldn't find any others that are for Hindu-Arabic numerals. Good point, I have to test what happens with languages with other numeral systems. Removing <setenv> is probably bad then.

How would you implement skipping such tests? Using <precheck>?

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 25, 2018

I tried with locales ps_AF.UTF-8 and prs_AF.UTF-8 and they both failed with removing setenv while succeeding before, so this cannot go in as-is.

This happens when building for native Windows (e.g. using MSVC or MinGW), but not when building for MSYS, so there's probably no good solution without running any code.

@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jul 25, 2018

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:test1148_decimal_comma branch from b773d87 to de7e7e9 Jul 26, 2018

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 26, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 26, 2018

That was fun! Something like this?

Unfortunately, Perl is not my native language and this is the first time I touched an automake file, so I'm not sure if I got this right. But it does work - the test is skipped in my German MSYS2 MinGW shell, where it previously failed, but not in my German MSYS2 MSYS shell, where it succeeds.

But, when I had chkdecimalpoint_SOURCES = chkdecimalpoint.c ../../lib/mprintf.c like many other test programs, I got errors about redefining the functions in mprintf.c without __declspec(dllexport) or something like this. I haven't managed to find out what I'm doing differently yet.
Also, I'm not entirely sure about the location of the helper program - chkhostname is located in libtest and used by non-lib curl tests, so I put it there too.

I still have to update potential other tests setting LC_NUMERIC as soon as this is OK.

@MarcelRaad MarcelRaad changed the title from test1148: use substitution for decimal sign to test1148: don't fail on Windows if decimal sign is not a point Jul 26, 2018

@MarcelRaad MarcelRaad changed the title from test1148: don't fail on Windows if decimal sign is not a point to test1148: don't fail on Windows if decimal separator is not a point Jul 26, 2018

@MarcelRaad MarcelRaad changed the title from test1148: don't fail on Windows if decimal separator is not a point to test1148: disable if decimal separator is not a point Jul 26, 2018

@gvanem

This comment has been minimized.

Member

gvanem commented Jul 27, 2018

I got errors about redefining the functions in mprintf.c without __declspec(dllexport) or something
like this.

Try adding -DBUILDING_LIBCURL while compiling chkdecimalpoint.c.

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:test1148_decimal_comma branch from de7e7e9 to fd9d309 Jul 27, 2018

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 27, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786
@dfandrich

This comment has been minimized.

Collaborator

dfandrich commented Jul 27, 2018

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 27, 2018

Thanks to both of you!

I wasn't sure if there was a difference between perl and C. But if perl-only works, that's better. I'll make sure it generates the same result and update the PR accordingly.

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 27, 2018

Unfortunately, that doesn't work because MSYS2's perl always respects LC_NUMERIC, probably because it's an MSYS program - there's only one perl in /usr/bin for both environments.

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:test1148_decimal_comma branch from fd9d309 to 02e0c8d Jul 28, 2018

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 28, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:test1148_decimal_comma branch from 02e0c8d to 571b161 Jul 29, 2018

MarcelRaad added a commit to MarcelRaad/curl that referenced this pull request Jul 29, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786
@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Jul 29, 2018

Fixed the MSVC static CMake build, breaking the autotools build. Fixed that, breaking the Linux CMake build 😃
I still don't understand what parts of Makefile.inc are used for CMake, the CPPFLAGS are not...

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes #2786

@MarcelRaad MarcelRaad force-pushed the MarcelRaad:test1148_decimal_comma branch from 571b161 to 837c6c9 Aug 15, 2018

@MarcelRaad

This comment has been minimized.

Member

MarcelRaad commented Aug 16, 2018

Finally all configurations are passing. I haven't found any other curl test setting LC_NUMERIC.

@MarcelRaad MarcelRaad deleted the MarcelRaad:test1148_decimal_comma branch Aug 24, 2018

xquery added a commit to xquery/curl that referenced this pull request Sep 3, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786

falconindy added a commit to falconindy/curl that referenced this pull request Sep 10, 2018

test1148: disable if decimal separator is not point
Modifying the locale with environment variables doesn't work for native
Windows applications. Just disable the test in this case if the decimal
separator is something different than a point. Use a precheck with a
small C program to achieve that.

Closes curl#2786

@lock lock bot locked as resolved and limited conversation to collaborators Nov 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.