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 9 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
100 changes: 94 additions & 6 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(__MINGW32__) || defined(__MINGW64__)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

typedef struct {
FILE *f;
int is_kind;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use an enum to be more readable please.

char filename[64];
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use JS__PATH_MAX to be safe?

Copy link
Author

Choose a reason for hiding this comment

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

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?

} JSTMPFile;
#endif


static bool is_stdio(FILE *f)
{
return f == stdin || f == stdout || f == stderr;
Expand All @@ -1044,17 +1053,26 @@ 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(__MINGW32__) || defined(__MINGW64__)
JSTMPFile *ss = (JSTMPFile*) s;
if (ss->is_kind==2) {
if (ss->f) fclose(ss->f);
if (ss->filename[0] != 0) remove(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);
}
}
};
};

static ssize_t js_get_errno(ssize_t ret)
{
Expand Down Expand Up @@ -1207,6 +1225,62 @@ static JSValue js_std_fdopen(JSContext *ctx, JSValueConst this_val,
}

#if !defined(__wasi__)
#if defined(__MINGW32__) || defined(__MINGW64__)
// c:/tmp no longer works in windows. c:/ doesn't work. permissions!
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;
obj = JS_NewObjectClass(ctx, ts->std_file_class_id);
if (JS_IsException(obj))
return obj;
s = js_mallocz(ctx, sizeof(*s));
if (!s) {
JS_FreeValue(ctx, obj);
return JS_EXCEPTION;
}

char * env = getenv("TMP");
if (!env) env = getenv("TEMP");
int i = 0;
if (env) {
while (env[i]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

s->filename[i] = env[i];
i++;
if (i > 50) return JS_NULL;
};
};
char* fname = &s->filename[i];
char* templ = "\\qXXXXXXX";
while (templ[0]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

memcpy?

fname[0] = templ[0];
fname++; templ++;
};
fname[0] = 0;
int mkf = mkstemp(s->filename);
if (mkf == -1) {
JS_FreeValue(ctx, obj);
js_free(ctx, s);
return JS_NULL;
};
int fd = dup(mkf);
s->f = fdopen( fd, "a+");
close(mkf);
if (argc >= 1) js_set_error_object(ctx, argv[0], s->f ? 0 : errno);
if (!s->f) {
JS_FreeValue(ctx, obj);
js_free(ctx, s);
return JS_NULL;
};

s->is_kind = 2;
JS_SetOpaque(obj, s);
return obj;
}
#else // MINGW
static JSValue js_std_tmpfile(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
{
Expand All @@ -1218,7 +1292,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 +3130,7 @@ static JSValue js_os_realpath(JSContext *ctx, JSValueConst this_val,
}
return make_string_error(ctx, buf, err);
}
#endif

#if !defined(_WIN32) && !defined(__wasi__)
static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
int argc, JSValueConst *argv)
{
Expand All @@ -3072,12 +3145,27 @@ static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
JS_FreeCString(ctx, target);
return JS_EXCEPTION;
}
#ifdef _WIN32
int isdirflag = 0; // might need to pass a value in for folders.
if (argc > 2) {
if (JS_ToInt32(ctx, &isdirflag, argv[2])) return JS_EXCEPTION;
};
printf("symbolic link: %s -> %s; isdir: %d", target, linkpath, isdirflag);
// 2 = SYMBOLIC_LINK_FLAG_ALLOW_UNPRIVILEGED_CREATE
Copy link
Contributor

Choose a reason for hiding this comment

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

Use the constant rather than the value?

Copy link
Author

Choose a reason for hiding this comment

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

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.

err = CreateSymbolicLinkA(linkpath, target, ( isdirflag | 2 ) ) ;
printf(" returned %d\r\n", err);
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
12 changes: 11 additions & 1 deletion tests/test_std.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,11 +168,19 @@ 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) {

// I bet this can be fixed in windows 10
assert(st.mtime, fdate);

/* symlink in windows 10+ requres admin or SeCreateSymbolicLinkPrivilege privilege found under:
Computer Configuration\Windows Settings\Security Settings\Local Policies\User Rights Assignment\
*/
err = os.symlink(fname, link_path);
assert(err, 0);


[st, err] = os.lstat(link_path);
assert(err, 0);
Expand Down Expand Up @@ -298,3 +306,5 @@ 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.

std.exit(0);
Loading