Win32 test std #1090

coreyapowell · 2025-05-31T02:05:11Z

tested in Win11 MSYS2 Mingw64, Clang64, WSL Debian, MSVC 2023, Clang in MSVC 2022
finished modifications so that test_std can pass audit in windows.
adds Win32 os_symlink
modifies std_tmpfile for MINGW
adds destructor to std_File for tmpfile deletion

this does not contain os.exec or inclusion of Win32 std_test.js for that. It didn't test windows before, and that can probably be moved from test_std (considering it's not std it's os anyway). my change is separated and that test script is separated in that change.
test_std no longer tries to create a symlink in windows because you need admin rights, but it works if you test it manually.
also there's a time bug returning negative number I didn't get into removed from test_std

I put this back like it was. Changes will be split into smaller parts and I've gotten confused when permissions went back and forth

Update test_std.js

getting back to original codebase

modifies std_tmpfile to work properly with MINGW (I think MSVC will work okay) std_file finalizer modified creates os_symlink for win32

moved a time issue out of WIN32 section. I moved os.symlink out as well and commented, you need admin rights or a setting to execute. test works now with these changes in Windows 11.

coreyapowell · 2025-05-31T20:36:49Z

I keep seeing "we're all done". Great! I don't know if I'm supposed to leave this branch in place or merge it in my main.

bnoordhuis

Same comment as the other PR, can you fix the style issues first? Thanks.

coreyapowell · 2025-06-02T01:46:26Z

Sure! I'll also pull out all of the comments and flags.

... okay, sorry, I took out more than comments. I had some printf functions left in there! I think that warrants some testing, that I didn't see that printf! The reason I didn't see that printf was that windows 10 requires admin rights to test symlinks, and I have it disabled in the test script. I've left a documented flag in test_std.js and properly tested this again. MING64, CLANG64, MSVC, ClangCL with VC Build

const IS_WIN_ADMIN_TEST_FLAG = 0; modified so that someone can run as admin with this flag on and actually test symlink. comment above that clarified that "this code is broken". - That's not my code, my code makes the majority of the std_file finish executing. I just can't readlink from windows , and I can't do that mtime. ...... (but I don't need metric time, I'm not a scientist!

*space*

...

coreyapowell · 2025-06-02T05:53:56Z

I didn't think I'd get to it tonight, but I've cleaned these files up. (just the tmpfile/symlink files)
I had accidentally posted some comments in the wrong thread earlier, if you got a message about the other code.

I don't know if you'd seen this, I had printf's still in the code for symlink..
You have to be an administrator in windows 10+ now, so I was manually moving that around in the test scripts. I put a flag in test_std.js that can be changed and execute the code. I'm glad I did because going back and forth between these environments and running all these compilers and test scripts things like that get missed! I don't know if you guys have a one-stop place to execute the tests, I'm manually executing the scripts in the test folder. And not all of them work.

That was the whole point of even fixing symlink in windows. I don't need it! I just wanted to see the test scripts complete. I might use tmpfile more.

Also, and I'm asking this. maybe I should post a bug discussion? The line before symlink, testing mtime, reports -1000 and expects 10000 or something on all 4 or my windows builds. That's broken too, and that's not something I immediately knew how to fix. So I've bypassed it in the test but it used to be there. I can get to it in the future if it's fixable, but I need this functionality updated so that I can use Quickjs to run my compiles and detect error codes and link my modules for me. I'm doing things the hard way for now!

saghul

Left some comments, PTAL.

saghul · 2025-06-03T10:36:24Z

quickjs-libc.c

@@ -1034,6 +1034,14 @@ typedef struct {
    bool is_popen;
 } JSSTDFile;

+#if defined(__MINGW32__) || defined(__MINGW64__)


We don't support the old MinGW32 so you can drop that part of the test.

saghul · 2025-06-03T10:36:42Z

quickjs-libc.c

+typedef struct {
+    FILE *f;
+    int is_kind; 
+    char filename[64];


Maybe use JS__PATH_MAX to be safe?

is using the tmp environment path size and malloc ... but I'm glad you told me about this. I can use that for os.exec command line buffer size?

saghul · 2025-06-03T10:37:01Z

quickjs-libc.c

+#if defined(__MINGW32__) || defined(__MINGW64__)
+typedef struct {
+    FILE *f;
+    int is_kind; 


Use an enum to be more readable please.

saghul · 2025-06-03T10:38:32Z

quickjs-libc.c

+        env = getenv("TEMP");
+    int i = 0;
+    if (env) {
+        while (env[i]) {


Make the buffer long enough or malloc it, this feels wrong.

now that you mention it, since I've gone to the trouble of a special destructor, malloc is the right answer. The first time around I was using a different function that handled deleting the file, but then something else was wrong with that solution. I'm glad Bill Gates is gone!

everyone's looking for a solution to this problem, it's actually not easy to get working. (that link wasn't what I thought). I maybe should be detecting the _WIN32 flag here because I'm not sure if Mingw in linux is going to to compile the wrong code here.

hey sir! do you suggest using malloc? or do you suggest using js_malloc? I was afraid of thrashing the JS memory but I didn't know if there's concerns with the application or the memory management that requires js_malloc.

saghul · 2025-06-03T10:38:54Z

quickjs-libc.c

+    }
+    char* fname = &s->filename[i];
+    char* templ = "\\qXXXXXXX";
+    while (templ[0]) {


saghul · 2025-06-03T10:41:36Z

quickjs-libc.c

@@ -1207,6 +1224,63 @@ static JSValue js_std_fdopen(JSContext *ctx, JSValueConst this_val,
 }

 #if !defined(__wasi__)
+#if defined(__MINGW32__) || defined(__MINGW64__)
+static JSValue js_std_tmpfile(JSContext *ctx, JSValueConst this_val,


Actually, why are we doing all of this when MinGW already has tmpfile ?

because it has been broken for years. It's hit and miss with microsoft compilers, because the internal windows code that MINGW calls is still there and that doesn't work. I researched it. It's a folder permission change in the operating system, in at least back to win10. None of my windows versions could run the test_std.js file because of that and the mtime problem.

Is this the problem? msys2/MINGW-packages#18878 (comment)

Perhaps it's time to default to the UCRT build.

This is pretty clean now. I just tested all 3 builds and there's no compiler warnings. I wouldn't do that!
The whole reason I am on this project helping get your code in order is this:
I followed Node before it was Node.js. Has it been 20 years? Then the Google team had me registering with Google Code. Then I had to do this Clang Build with the MS tools. The requirements became too strict, and then I had to use their custom build scripts. They broke with everything I had. They would not support Mingw and Clang64 on Msys2. It used to work! I have other software that would not support the UCRT, so I can't do both.

tests/test_std.js

saghul · 2025-06-03T10:42:54Z

quickjs-libc.c

@@ -3072,12 +3146,25 @@ static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
        JS_FreeCString(ctx, target);
        return JS_EXCEPTION;
    }
+#ifdef _WIN32
+    int isdirflag = 0; 


What is this used for?

Well you need to tell me if you want the comment or how you want it documented. it is a flag that can be passed if you want to create a directory link. Above the function I commented:
/* in WIN32: (0 | err) os.symlink(target, linkpath, bool isDirectory)
@ msdn: linkpath and target have reversed meaning than symlink! */

The line below it was supposed to be: err = CreateSymbolicLinkA(linkpath, target, ( isdirflag | 2 ) ) ;
but I thought it was a flag. I took it out. I tested that manually before and didn't add folder symlink testing to test_std.js
To be honest it should also have a comment: 1 = SYMBOLIC_LINK_FLAG_DIRECTORY if it's left in there.
Also there probably should also be testing added to test_std.js for folders. It will create a broken link and I didn't get readlink working.

I'd say we can leave directories out (or stat and check if it's a directory and do the right thing without pushing the responsibility to the user)

if you want the symlink taken out, it's not exactly the same and error prone in windows. I don't need it. It's more of a shortcut I think, and it doesn't warn you if the path isn't real or anything. You have to be an admin to use it. I just wanted to run test_std.js and get it to finish.

it's a shortcut, and fopen doesn't even work on it. I don't know if it's worth it.

You've left the change request. Is that for this?
I programmed it as an optional parameter, so if a user doesn't specify the flag parameter, it then it still creates a file link by default. I will actually use this, for instance, when I download the current release and link test262 (rather than copy 2GB). Windows users are used to this restriction honestly. To me, qjs could be a first class admin tool.

saghul · 2025-06-03T10:43:39Z

quickjs-libc.c

+    if (argc > 2) {
+         if (JS_ToInt32(ctx, &isdirflag, argv[2])) return JS_EXCEPTION;
+    }
+    // 2 = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE


Use the constant rather than the value?

I had an error that it couldn't find header and just left it in the comment.
I've reinstalled my Windows LLVM setup since then, the error may have gone away. Lots of bugs have gone away with Clang 20. I'm making sure now that it's there. That version was built when there was way less support, back when V8 added Clang as a build environment. That was before you could just add Clang from the Visual Studio menu.

altercation: MINGW64 ... no longer MINGW32 no longer duping the tmpfile handle. that was because of some other function and it's not needed. improved os_tmpfile names of variables for clarity. JSTMPFile stores a js_mallocz'd buffer instead. frees buffer. enumerated JSSTDFileKind kind;

fixed flag: SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE tested

coreyapowell · 2025-06-03T17:53:46Z

thank you for the opportunity of getting this in here. changes made.

enummbed JSTMPFileKind. (it's just JSTMPFile, I didn't change JSSTDFile. I don't know who's project that affects. But the bool should be 1 for a pipe in the old code. It should work.
fixed the symbolic_link flag and restored the isdirflag | symbolic_link
get tmp folder size and malloc filename.
stores filename as a js_mallocz buffer in JSTMPFile.
I no longer dup the tmpfile handle. I just fopen the FD mkstemp gives me.
clarified the variables a bit.
no longer testing std.exit()

so the best solutions to the symlink dir issue online are to open the raw shortcut or use kernel level functions to read the harddrive node directly. And I'm not getting paid for any of this, so that's not free, my laptop's not insured. If you want to learn the file format of a shortcut in windows, I'm sure that will work until windows 12 comes along.

I have to manually test this change unless we add a detect user or detect group function. Win32 has no lstat. Moved the comment and admin flag to the top for ease of use until someone comes up with a better solution.

saghul · 2025-06-05T07:40:27Z

quickjs-libc.c

+            if (ss->f) fclose(ss->f);
+            if (ss->filename != 0) remove(ss->filename);
+            js_free_rt(rt, ss->filename);
+        } else if (s->f && !is_stdio(s->f)) {


I think you can just return here rather than having the check duplicated

Do you mean return and take the else out?

Pretty much. Well, you may need some extra cleanup, or a goto end...

yes I left it open for cleanup before.
Added 'return;' and took out 'else'. I just wasn't sure what you meant.

saghul · 2025-06-05T07:41:08Z

quickjs-libc.c

+        JS_FreeValue(ctx, obj);
+        return JS_EXCEPTION;
+    }
+    fn_template = "\\qXXXXXXX";


Make this static?

made static. I always assumed the compiler was smart enough to do that. So I researched it and I get it, someone might want their strings on their stack, so the default is not.
I have always seen it in the string table, not on the stack. Researching it, maybe my compilers were optimizing that way, and I just assumed it was static by default. Maybe I'm thinking of a const string.

saghul · 2025-06-05T07:42:17Z

quickjs-libc.c

+        env_tmp = getenv("TEMP");
+    if (env_tmp) {
+        fn_len = strlen(env_tmp);
+        fn_buff = js_malloc(ctx, fn_len + 10);


don't use 10, if that is the length of the template, use strlen

used strlen, but with a lot of addition around it.
I hated to malloc 10. That's why this got so busy.
And I memcpy the 0.

saghul · 2025-06-05T07:43:56Z

quickjs-libc.c

+        memcpy(fn_buff, env_tmp, fn_len);
+        memcpy(fn_buff + fn_len, fn_template, 10);
+    } else {
+        fn_buff = js_mallocz(ctx, 10);


You can share the allocation and copy code across both branches by checking if you have an env var or not.

It's wrong anyway. Even if there's no %TEMP% I put "/" and that would default to c:. That defeats the whole point. There are no create permissions there. I should throw an exception if I don't have TEMP. Otherwise I should have added "\" manually.
Let me know if you disagree. My original logic was to create in CWD if someone's stripped the environment.

There's no way around two ifs that I could see.
I admit, may be clearer for someone else to read the way you're suggesting.

saghul · 2025-06-05T07:45:25Z

quickjs-libc.c

+    };
+    //int fd = dup(mkf);
+    s->f = fdopen( mk_fd, "a+");
+    if (s->f == 0) {


Check if (!s->f

saghul · 2025-06-05T07:48:46Z

quickjs-libc.c

+    }
+    err =  CreateSymbolicLinkA(linkpath, target, ( isdirflag | 
+        SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE ) ) ; 
+    if (!err) err = GetLastError();


style: please use similar style to what we use all across the codebase

saghul · 2025-06-05T07:51:06Z

tests/test_std.js

+/* symlink in windows 10+ requres admin or SeCreateSymbolicLinkPrivilege privilege found under:
+ Computer Configuration\Windows Settings\Security Settings\Local Policies\User Rights Assignment\
+ set IS_WIN_ADMIN_TEST_FLAG to 1 and run as Administrator to test win32 os.symlink */
+const IS_WIN_ADMIN_TEST_FLAG = 0;


Rather than do this, check the error code and if it's a permission error then don't fail the test.

removed.
as per other comment, it's making less sense that windows has done this to the feature.

saghul · 2025-06-05T07:51:39Z

tests/test_std.js

@@ -168,9 +172,12 @@ function test_os()
    [st, err] = os.stat(fpath);
    assert(err, 0);
    assert(st.mode & os.S_IFMT, os.S_IFREG);
-    assert(st.mtime, fdate);
+
+    if (!isWin) // returns some negative value in windows 11. had to remove this on my build. (CAP)


Please undo this unrelated change, our test suite does not reproduce the problem.

I'll fix it, but this took me an hour to solve. it might be something in windows 11:

Error: assertion failed: got |-1000|, expected |10000|

I finally was able to verify the bug:

fdate = 18000000; err = os.utimes(fpath, fdate, fdate);

you see anything before that was 1969 according to my timezone, and windows 11 would not handle it. That's in Windows 11 on all 4 different builds I'm doing. So my question in response is: what versions of Windows do you verify against? This could be a timezone issue.
There is no error to this. MSDN claims _time will return -1 on error, but it seems like something internal is getting -1 and setting that into the internal structure. Later it's stat which sees -1 and converts it to milliseconds. The linux version calls a different function that doesn't modify the time on failure.

You can check the CI taml file, we check against a few windows versions, 11 is amongst them.

Even if there is a timezone issue, it's not related to this PR so it should be handled in a different one.

thanks. I had a minute and figured it out. It just glitches out to set that time before 5am here in NY time. That's 18 million milliseconds. The script needs more than 10,000.
I put 86 million 400 thousand milliseconds, which is 24 hours, which gets any user through Dec 31 1969 to Jan 1.

saghul · 2025-06-05T07:52:14Z

tests/test_std.js

@@ -183,6 +190,14 @@ function test_os()
        assert(buf, fname);

        assert(os.remove(link_path) === 0);
+
+    } else if (IS_WIN_ADMIN_TEST_FLAG) {


As mentioned above, handle the failure gracefully on Windows.

So we start the test and skip the test? I'll do that, but would you rather remove it at this point?
I know I said I'd use the feature, but now I'm using hard links. I will be frank with you, lstat needs to be working for all of this and that's too large of a change for now.

Fine by me, we can handle one change at a time. Keep this PR to tmpfile alone then?

I just exit gracefully or remove the link. I suppose you need to test removing the link.

sorry you commented as I had the screen un-refreshed. It looked ugly to me when I commented but it's growing on me. Silence is golden. It's whatever you want.
If someone is editing the file to test it, they can put a console.log in to see it. I removed the comment, and I just hate for another windows user to learn this the hard way.
Coincidentally this isn't a problem with hard links. I'm doing hard links in my batch file when I download the git and move test262 and it doesn't complain. If os.exec is fixed, I would just use that. I'm pretty sure that symlink isn't a hard link, right? That kind of makes Microsoft's decision silly.

saghul · 2025-06-05T07:52:28Z

tests/test_std.js

@@ -298,3 +313,4 @@ test_interval();
 test_timeout();
 test_timeout_order();
 test_stdio_close();
+


Drop the extra line.

changes in as requested and tested.
I will download and make sure after I've had a break.

removed extra line. win32 symlink fails on 1314 because admin rights required. removed flag.

cleaned up js_os_tmpfile cleaned up finalizer

fixed indention level

coreyapowell · 2025-06-05T20:12:18Z

requested changes made.
downloaded from git and executed tests.
executed test262 on debian.

I've updated the merged copy of these two changes and I work from that, for my stuff. And I'm about a step away from having the windows version running test262. I used your cmake but I did manage to get Mingw64 to run it.

I've crashed the board! Maybe removing the symlink makes more sense for now. If I work on this any further, it's going to be my own cross compatible version of lstat done initially as a c header.

coreyapowell added 9 commits May 29, 2025 16:02

Add files via upload

6ee553f

Add files via upload

f323678

Add files via upload

7774b71

Add files via upload

ba9e4c4

Update test_std.js

3d43bdb

I put this back like it was. Changes will be split into smaller parts and I've gotten confused when permissions went back and forth

Merge pull request #2 from coreyapowell/backto_original

60c56b1

Update test_std.js

Delete test_std.js

a5395d3

getting back to original codebase

Update quickjs-libc.c

e7e1a1f

modifies std_tmpfile to work properly with MINGW (I think MSVC will work okay) std_file finalizer modified creates os_symlink for win32

Update test_std.js

18baa91

moved a time issue out of WIN32 section. I moved os.symlink out as well and commented, you need admin rights or a setting to execute. test works now with these changes in Windows 11.

bnoordhuis reviewed Jun 1, 2025

View reviewed changes

coreyapowell added 4 commits June 2, 2025 01:22

Update quickjs-libc.c

e91b646

*space*

Update quickjs-libc.c

330df2a

...

saghul requested changes Jun 3, 2025

View reviewed changes

coreyapowell added 3 commits June 3, 2025 10:43

Update test_std.js

7b68bff

Update quickjs-libc.c

f29e7b5

fixed flag: SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE tested

Update test_std.js - no lstat

8d8b524

I have to manually test this change unless we add a detect user or detect group function. Win32 has no lstat. Moved the comment and admin flag to the top for ease of use until someone comes up with a better solution.

saghul requested changes Jun 5, 2025

View reviewed changes

coreyapowell added 3 commits June 5, 2025 14:41

not admin in win32 just skip symlink tests.

ec31538

removed extra line. win32 symlink fails on 1314 because admin rights required. removed flag.

tested and cleaned up

68ec872

cleaned up js_os_tmpfile cleaned up finalizer

formatting

8988eca

fixed indention level

Win32 test std #1090

Are you sure you want to change the base?

Win32 test std #1090

Conversation

coreyapowell commented May 31, 2025

Uh oh!

coreyapowell commented May 31, 2025

Uh oh!

bnoordhuis left a comment

Choose a reason for hiding this comment

Uh oh!

coreyapowell commented Jun 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coreyapowell commented Jun 2, 2025

Uh oh!

saghul left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell commented Jun 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coreyapowell commented Jun 2, 2025 •

edited

Loading

coreyapowell Jun 3, 2025 •

edited

Loading

coreyapowell Jun 3, 2025 •

edited

Loading

coreyapowell Jun 3, 2025 •

edited

Loading

coreyapowell Jun 5, 2025 •

edited

Loading

coreyapowell Jun 5, 2025 •

edited

Loading

coreyapowell Jun 5, 2025 •

edited

Loading