Skip to content

Win32 test std #1090

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.

Sign up for GitHub

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

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
+105 −8
Open

Win32 test std #1090

Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
6ee553f
Add files via upload
coreyapowell May 29, 2025
f323678
Add files via upload
coreyapowell May 29, 2025
7774b71
Add files via upload
coreyapowell May 29, 2025
ba9e4c4
Add files via upload
coreyapowell May 29, 2025
3d43bdb
Update test_std.js
coreyapowell May 30, 2025
60c56b1
Merge pull request #2 from coreyapowell/backto_original
coreyapowell May 30, 2025
a5395d3
Delete test_std.js
coreyapowell May 30, 2025
e7e1a1f
Update quickjs-libc.c
coreyapowell May 31, 2025
18baa91
Update test_std.js
coreyapowell May 31, 2025
11e0daa
took out some comments
coreyapowell Jun 2, 2025
e402f19
fixed comments and proper symlink test flag
coreyapowell Jun 2, 2025
e91b646
Update quickjs-libc.c
coreyapowell Jun 2, 2025
330df2a
Update quickjs-libc.c
coreyapowell Jun 2, 2025
7b68bff
Update test_std.js
coreyapowell Jun 3, 2025
4c04f21
Update quickjs-libc.c
coreyapowell Jun 3, 2025
f29e7b5
Update quickjs-libc.c
coreyapowell Jun 3, 2025
8d8b524
Update test_std.js - no lstat
coreyapowell Jun 4, 2025
ec31538
not admin in win32 just skip symlink tests.
coreyapowell Jun 5, 2025
68ec872
tested and cleaned up
coreyapowell Jun 5, 2025
8988eca
formatting
coreyapowell Jun 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
98 changes: 93 additions & 5 deletions quickjs-libc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1034,6 +1034,15 @@ typedef struct {
bool is_popen;
} JSSTDFile;

#if defined(__MINGW64__)
enum JSSTDFileKind { STDFile, STDPipe, TMPFile };
typedef struct {
FILE *f;
enum JSSTDFileKind kind;
char *filename;
} JSTMPFile;
#endif

static bool is_stdio(FILE *f)
{
return f == stdin || f == stdout || f == stderr;
Expand All @@ -1044,13 +1053,23 @@ static void js_std_file_finalizer(JSRuntime *rt, JSValueConst val)
JSThreadState *ts = js_get_thread_state(rt);
JSSTDFile *s = JS_GetOpaque(val, ts->std_file_class_id);
if (s) {
#if defined(__MINGW64__)
JSTMPFile *ss = (JSTMPFile*) s;
if (ss->kind == TMPFile) {
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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean return and take the else out?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@coreyapowell coreyapowell Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

#else
if (s->f && !is_stdio(s->f)) {
#endif
#if !defined(__wasi__)
if (s->is_popen)
pclose(s->f);
else
#endif
fclose(s->f);

}
js_free_rt(rt, s);
}
Expand Down Expand Up @@ -1207,6 +1226,62 @@ static JSValue js_std_fdopen(JSContext *ctx, JSValueConst this_val,
}

#if !defined(__wasi__)
#if defined(__MINGW64__)
static JSValue js_std_tmpfile(JSContext *ctx, JSValueConst this_val,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@coreyapowell coreyapowell Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

int argc, JSValueConst *argv)
{
JSRuntime *rt = JS_GetRuntime(ctx);
JSThreadState *ts = js_get_thread_state(rt);
JSTMPFile *s;
JSValue obj;
char* fn_template, *env_tmp, *fn_buff;
int mk_fd, fn_len;

obj = JS_NewObjectClass(ctx, ts->std_file_class_id);
if (JS_IsException(obj))
return JS_EXCEPTION;
s = js_mallocz(ctx, sizeof(*s));
if (!s) {
JS_FreeValue(ctx, obj);
return JS_EXCEPTION;
}
fn_template = "\\qXXXXXXX";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make this static?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

env_tmp = getenv("TMP");
if (!env_tmp)
env_tmp = getenv("TEMP");
if (env_tmp) {
fn_len = strlen(env_tmp);
fn_buff = js_malloc(ctx, fn_len + 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

memcpy(fn_buff, fn_template, 10);
}
mk_fd = mkstemp(fn_buff);
if (mk_fd == -1) {
JS_ThrowInternalError(ctx, "tmpfile failed to create file with error: %d.", errno);
goto file_failure;
};
//int fd = dup(mkf);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the comment.

s->f = fdopen( mk_fd, "a+");
if (s->f == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if (!s->f

JS_ThrowInternalError(ctx, "tmpfile failed to open file with error: %d.", errno);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are leaking mk_fd here, you need to close it.

Copy link
Author

@coreyapowell coreyapowell Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I missed that. This got involved. I tried several fixes and found myself going in circles, and I'll tell you why:
I don't manually handle closing this file. Giving the user the FILE object was just confusing me to no end. But I get it now, once the FILE is in the managed structure, ownership is passed to the finalizer. If they had receive an int for a descriptor, closing the file is their problem. When they receive a FILE, Javascript handles that for them with the finalizer. Otherwise, should we be detecting multiple closes or is it safe to have redundant closes? It's not safe to close descriptors twice. Descriptors are reused.
It's confusing because I'm also writing my Socket library, and accept returns a Handle. I want to pass the Socket to a worker, so I want an int, but in other cases I create a Socket object from the handle. I am going in circles about how to handle this, and every time I get into this part of the code I think about all of this.
Also, all of the examples of the temp file code use dup on that descriptor and then close it. I see no reason to dup that descriptor.
But I understand you, that the throw should close it, and I missed it. I just want to make sure I'm not missing something in my logic. I'm probably overthinking the problem.

goto file_failure;
};
s->filename = fn_buff;
s->kind = TMPFile;
if (argc >= 1)
js_set_error_object(ctx, argv[0], s->f ? 0 : errno);
JS_SetOpaque(obj, s);
return obj;
file_failure:
JS_FreeValue(ctx, obj);
js_free(ctx, s);
js_free(ctx, fn_buff);
return JS_EXCEPTION;
}
#else // MINGW
static JSValue js_std_tmpfile(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
{
Expand All @@ -1218,7 +1293,8 @@ static JSValue js_std_tmpfile(JSContext *ctx, JSValueConst this_val,
return JS_NULL;
return js_new_std_file(ctx, f, false);
}
#endif
#endif // !MINGW
#endif // WASI

static JSValue js_std_sprintf(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
Expand Down Expand Up @@ -3055,9 +3131,8 @@ static JSValue js_os_realpath(JSContext *ctx, JSValueConst this_val,
}
return make_string_error(ctx, buf, err);
}
#endif

#if !defined(_WIN32) && !defined(__wasi__)
/* in WIN32: (0 | err) os.symlink(target, linkpath, bool isDirectory)
@ msdn: linkpath and target have reversed meaning than symlink! */
static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
{
Expand All @@ -3072,12 +3147,25 @@ static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
JS_FreeCString(ctx, target);
return JS_EXCEPTION;
}
#if defined(_WIN32)
int isdirflag = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this used for?

Copy link
Author

@coreyapowell coreyapowell Jun 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (argc > 2) {
if (JS_ToInt32(ctx, &isdirflag, argv[2])) return JS_EXCEPTION;
}
err = CreateSymbolicLinkA(linkpath, target, ( isdirflag |
SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE ) ) ;
if (!err) err = GetLastError();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

else err = 0;
#else
err = js_get_errno(symlink(target, linkpath));
#endif
JS_FreeCString(ctx, target);
JS_FreeCString(ctx, linkpath);
return JS_NewInt32(ctx, err);
}
#endif

#if !defined(_WIN32) && !defined(__wasi__)
/* return [path, errorcode] */
static JSValue js_os_readlink(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
Expand Down Expand Up @@ -4113,10 +4201,10 @@ static const JSCFunctionListEntry js_os_funcs[] = {
JS_CFUNC_DEF("sleep", 1, js_os_sleep ),
#if !defined(__wasi__)
JS_CFUNC_DEF("realpath", 1, js_os_realpath ),
JS_CFUNC_DEF("symlink", 2, js_os_symlink ),
#endif
#if !defined(_WIN32) && !defined(__wasi__)
JS_CFUNC_MAGIC_DEF("lstat", 1, js_os_stat, 1 ),
JS_CFUNC_DEF("symlink", 2, js_os_symlink ),
JS_CFUNC_DEF("readlink", 1, js_os_readlink ),
JS_CFUNC_DEF("exec", 1, js_os_exec ),
JS_CFUNC_DEF("getpid", 0, js_os_getpid ),
Expand Down
18 changes: 17 additions & 1 deletion tests/test_std.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ import { assert } from "./assert.js";
const isWin = os.platform === 'win32';
const isCygwin = os.platform === 'cygwin';

/* 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


function test_printf()
{
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@coreyapowell coreyapowell Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@coreyapowell coreyapowell Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

assert(st.mtime, fdate);

if (!isWin) {

err = os.symlink(fname, link_path);
assert(err, 0);

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

assert(os.remove(link_path) === 0);

} else if (IS_WIN_ADMIN_TEST_FLAG) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, handle the failure gracefully on Windows.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Author

@coreyapowell coreyapowell Jun 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


err = os.symlink(fname, link_path);
assert(err, 0);

assert(os.remove(link_path) === 0);

}

[buf, err] = os.getcwd();
Expand Down Expand Up @@ -298,3 +313,4 @@ test_interval();
test_timeout();
test_timeout_order();
test_stdio_close();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Drop the extra line.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading