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 all 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
103 changes: 98 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);
return;
};
#endif
if (s->f && !is_stdio(s->f)) {
#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,67 @@ 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 *env_tmp, *fn_buff;
int mk_fd, path_len, fn_template_len, fn_total_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;
}
static char* fn_template = "qXXXXXXX";
fn_template_len = strlen(fn_template) + 1;
fn_total_len = fn_template_len;
path_len = 0;
env_tmp = getenv("TMP");
if (!env_tmp)
env_tmp = getenv("TEMP");
if (env_tmp) {
path_len = strlen(env_tmp);
fn_total_len = path_len + 1 + fn_template_len;
}
fn_buff = js_malloc(ctx, path_len + fn_total_len);
if (env_tmp) {
memcpy(fn_buff, env_tmp, path_len);
fn_buff[path_len] = '\\';
path_len++;
}
memcpy(fn_buff + path_len, fn_template, fn_template_len);
mk_fd = mkstemp(fn_buff);
if (mk_fd == -1) {
JS_ThrowInternalError(ctx, "tmpfile failed to create file with error: %d.", errno);
goto file_failure;
};
s->f = fdopen( mk_fd, "a+");
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.

close(mk_fd);
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 +1298,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 +3136,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 +3152,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 +4206,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
10 changes: 7 additions & 3 deletions tests/test_std.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { assert } from "./assert.js";
const isWin = os.platform === 'win32';
const isCygwin = os.platform === 'cygwin';


function test_printf()
{
assert(std.sprintf("a=%d s=%s", 123, "abc"), "a=123 s=abc");
Expand Down Expand Up @@ -160,7 +159,7 @@ function test_os()
assert(err, 0);
assert(files.indexOf(fname) >= 0);

fdate = 10000;
fdate = 86400000;

err = os.utimes(fpath, fdate, fdate);
assert(err, 0);
Expand All @@ -170,8 +169,9 @@ function test_os()
assert(st.mode & os.S_IFMT, os.S_IFREG);
assert(st.mtime, fdate);

err = os.symlink(fname, link_path);

if (!isWin) {
err = os.symlink(fname, link_path);
assert(err, 0);

[st, err] = os.lstat(link_path);
Expand All @@ -183,6 +183,10 @@ function test_os()
assert(buf, fname);

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

} else if (err != 1314) {
assert(err, 0);
assert(os.remove(link_path) === 0);
}

[buf, err] = os.getcwd();
Expand Down