Re: [Tinycc-devel] test 104 fails on windows: missing mkstemps

[Petr Skočík]

The test started out as a shell script. It could have been a pipeline,
but what I dislike about pipelines in POSIX shell is you can't
really error check them well because if a link fails and the last one
succeeds, you get a success.

That is why I wrote I ended up invoking the links from C
using tempfiles as glue. You can theoretically do the same from a shell
but mktemp isn't standard but you can create tempfiles from C standardly
(mkstemps() is POSIX, tmpnam() is ISO C) and the C code isn't much
longer than the shell code.

Petr

On 6/19/19 11:33 AM, avih wrote:
> Hi Christian and thanks for the quick response.
> 
> I can confirm that your patch fixes the issue (makes the diff go away).
> 
> However, before the patch it definitely invoked the cygwin sort because
> my PATH only has cygwin's /bin and /local/bin (translated to correct
> windows paths when test 104 executes) and prefixed with TCC's root path.
> I tested this by printing getenv("PATH") at 104_inline_test.c .
> 
> I can also confirm the issue simply from cygwin's shell that `sort`
> changes the order of 104_inline_test.expect:
> $ cat tests/tests2/104_inline_test.expect | sort # <-- inst_... is
> before inst2_... i.e. MODIFIED.
> 
> while
> 
> $ cat tests/tests2/104_inline_test.expect | LC_ALL=C sort # <-- inst2...
> is before inst_...  i.e. unmodified = OK
> 
> Alternative to your patch, adding `export LC_ALL = C` at the beginning
> of tests/tests2/Makefile also fixes the issue for me.
> 
> Running `locale` at the shell prints this for me:
> $ locale
> LANG=en_US.UTF-8
> LC_CTYPE="en_US.UTF-8"
> LC_NUMERIC="en_US.UTF-8"
> LC_TIME="en_US.UTF-8"
> LC_COLLATE="en_US.UTF-8"
> LC_MONETARY="en_US.UTF-8"
> LC_MESSAGES="en_US.UTF-8"
> LC_ALL=
> 
> Please don't push this patch to mob, because it adds another potential
> complexity by mixing unix/windows tools.
> 
> I'll try to modify 104_inline_test.c such that it uses only one system
> invocation of shell -c "<commands>" so that it effectively becomes a
> shell script, and post the result of my attempt here to get some feedback.
> 
> Thanks again,
> Avi
> 
> 
> On Wednesday, June 19, 2019 8:20 AM, Christian Jullien
> <address@hidden> wrote:
> 
> 
> Avih,
> I quickly hacked 104 test to start to make it work but I’m not the
> author of this test. I let Petr improve it.
> I fully agree with you that mixing C code and calling system (which
> forks a cmd.exe on Windows) to finally execute gnu commands that require
> Cygwin is a BAD idea!!
> As you said, it’s much easier if 104 test generates a .o which the whole
> ‘unix’ infrastructure test will check. I let the maintainer of this test
> decide what to do.
>  
> About the diff, I don’t understand quite well what happens. Among other,
> it calls system(“sort …”); which spawns cmd.exe and then execute first
> sort seen in path. Depending on your PATH it can be Windows sort (if
> Windows path comes first) or Cygwin version if this one comes first.
>  
> Can you please try this diff which works as good for me but now forces
> sort Windows version:
>  
> diff --git a/tests/tests2/104_inline_test.c b/tests/tests2/104_inline_test.c
> index cb288d2..d191602 100644
> --- a/tests/tests2/104_inline_test.c
> +++ b/tests/tests2/104_inline_test.c
> @@ -5,8 +5,8 @@
>  
> #if __linux__ || __APPLE__
> #define SYS_WHICH_NM "which nm >/dev/null 2>&1"
> +#define SYS_SORT     "sort"
> #define TCC_COMPILER "../../tcc"
> -#define SYS_AWK
>  
> char c[]="/tmp/tcc-XXXXXX"; char o[]="/tmp/tcc-XXXXXX";
> static int mktempfile(char *buf)
> @@ -15,6 +15,7 @@ static int mktempfile(char *buf)
> }
> #elif defined(_WIN32)
> #define SYS_WHICH_NM  "which nm >nul 2>&1"
> +#define SYS_SORT      "%systemroot%\\system32\\sort /LOCALE C"
>  
> #if defined(_WIN64)
> #define TCC_COMPILER "..\\..\\win32\\x86_64-win32-tcc"
> @@ -72,7 +73,7 @@ int main(int C, char **V)
>          sprintf(buf, "%s -c -xc %s -o %s", V[1]?V[1]:TCC_COMPILER, c,
> o); if(0!=system(buf)){ r=1;goto out;}
>          sprintf(buf, "nm -Ptx %s > %s", o, c); if(system(buf))
> {r=1;goto out;}
>          sprintf(buf, "gawk '{ if($2 == \"T\") print $1 }' %s > %s", c,
> o); if(system(buf)) {r=1;goto out;}
> -        sprintf(buf, "sort %s", o); if(system(buf)) {r=1;goto out;}
> +        sprintf(buf, "%s %s", SYS_SORT, o); if(system(buf)) {r=1;goto out;}
> out:
>         remove(c);
>         remove(o);
>  
> *From:*Tinycc-devel
> [mailto:tinycc-devel-bounces+eligis=address@hidden] *On Behalf Of
> *avih
> *Sent:* Tuesday, June 18, 2019 21:01
> *To:* address@hidden; address@hidden; 'Michael Matz'
> *Subject:* Re: [Tinycc-devel] test 104 fails on windows: missing mkstemps
>  
> Well, the diffs are not really diffs. They just moved. Looks like `sort`
> didn't work as expected, or maybe it's some locale issue (mine is
> en_US.UTF-8 at cygwin, and en-US at windows).
>  
> A script should handle this too, possibly with LC_ALL=C (and make sure
> the reference file is generated with the same sort locale).
>  
> If someone could split it to program+script then I can test the test in
> various use cases and make adjustment if required (I'm terrible with
> Makefiles but reasonably useful with shell).
>  
> On Tuesday, June 18, 2019 9:50 PM, avih <address@hidden> wrote:
>  
> After commit d39c49db  Remove empty conditional _WIN32 code
> 
> 
> and some hacking of the code (it's an unhealthy mix of basically running
> a shell script from a program compiled using tcc for windows), I get the
> following 2 diffs:
> 
> 
> +inst_extern_inline_postdeclared
> +inst_extern_inline_predeclared
> 
> 
> 
> and
> 
> 
> -inst_extern_inline_postdeclared
> -inst_extern_inline_predeclared
> 
> 
> 
> I'm running it in a cygwin environment and the tools (nm, sort, gawk)
> are cygwin tools, while the tested tcc is normal mingw tcc for windows
> (which I build in a reproducible way using my script).
> 
> 
> 
> Regardless of these two diffs, I think the test should be composed of a
> program and a normal shell script (which uses mktemp, awk, sort etc), so
> that the paths are consistent between the tools.
> 
> 
> Also, the TCC path is hardcoded at the test, but in fact it's parametric
> at the makefile as $(TCC), so it's better to use that instead (but then
> there are forward/backward slash issues which need to be handled too,
> because system(...) in win32 expects backward slashes, but $(TCC) at the
> makefile has forward slashes). Making this a program + a script should
> implicitly solve this issue as well.
> 
> 
> 
> After all, a working shell+tools is assumed for this test anyway, but
> the current way of using system(...) from a win32 program (compiled
> using tcc for windows) invokes a windows shell which can be inconsistent
> with the actual shell where `make` runs.
> 
> 
> Avi
> 
> 
> 
> 
> 
>  
> On Tuesday, June 18, 2019 12:11 AM, avih <address@hidden> wrote:
>  
> Hmm.. I now see that test 104 uses signal and nm, so it might require
> some effort to make it work on windows.
>  
> Nevertheless, considering the recent breakage specifically on windows
> from related code, I think it would be beneficial if this test could be
> made to work on windows,
>  
> On Monday, June 17, 2019 11:54 PM, avih <address@hidden> wrote:
>  
> Wouldn't it be better to just create a known/fixed file instead?
> (assuming the test doesn't need explicitly mkstemps, I didn't look at
> its actual code).
>  
> On Monday, June 17, 2019 11:50 PM, Christian Jullien <address@hidden>
> wrote:
>  
> Yes it has been previously reported.
>  
> Michael, as said in a private mail, I agree with you that this test
> should be skipped on Windows.
>  
> C.
>  
> *From:*Tinycc-devel
> [mailto:tinycc-devel-bounces+eligis=address@hidden] *On Behalf Of
> *avih
> *Sent:* Monday, June 17, 2019 22:46
> *To:* Tinycc-devel Mailing List
> *Subject:* [Tinycc-devel] test 104 fails on windows: missing mkstemps
>  
> Test 104 log on windows (both tcc32 and tcc 64):
>  
> Test: 104_inline_test...
> --- 104_inline_test.expect      2019-06-17 23:42:00.162697100 +0300
> +++ 104_inline_test.output      2019-06-17 23:42:35.531550400 +0300
> @@ -1,25 +1,2 @@
> -extern_extern_postdeclared
> -extern_extern_postdeclared2
> -extern_extern_predeclared
> -extern_extern_predeclared2
> -extern_extern_prepostdeclared
> -extern_extern_prepostdeclared2
> -extern_extern_undeclared
> -extern_extern_undeclared2
> -extern_postdeclared
> -extern_postdeclared2
> -extern_predeclared
> -extern_predeclared2
> -extern_prepostdeclared
> -extern_undeclared
> -extern_undeclared2
> -inst2_extern_inline_postdeclared
> -inst2_extern_inline_predeclared
> -inst3_extern_inline_predeclared
> -inst_extern_inline_postdeclared
> -inst_extern_inline_predeclared
> -main
> -noinst_extern_inline_func
> -noinst_extern_inline_postdeclared
> -noinst_extern_inline_postdeclared2
> -noinst_extern_inline_undeclared
> +104_inline_test.c:30: warning: implicit declaration of function 'mkstemps'
> +tcc: error: undefined symbol 'mkstemps'
> make[1]: *** [Makefile:70: 104_inline_test.test] Error 1
> Test: 105_local_extern...
> make[1]: Target 'all' not remade because of errors.
>  
>  
>  
>  
>  
> 
>