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 13 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
97 changes: 92 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,14 @@ 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,13 +1052,22 @@ 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);
}
Expand Down Expand Up @@ -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,
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,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 +3146,25 @@ static JSValue js_os_symlink(JSContext *ctx, JSValueConst this_val,
JS_FreeCString(ctx, target);
return JS_EXCEPTION;
}
#ifdef _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;
}
// 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, ( 2 ) ) ;
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 +4200,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
23 changes: 22 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,9 +168,16 @@ 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);

/* symlink in windows 10+ requres admin or SeCreateSymbolicLinkPrivilege privilege found under:
Computer Configuration\Windows Settings\Security Settings\Local Policies\User Rights Assignment\
*/
const IS_WIN_ADMIN_TEST_FLAG = 0;
if (!isWin) {

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

Expand All @@ -183,6 +190,18 @@ 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);

[st, err] = os.lstat(link_path);
assert(err, 0);
assert(st.mode & os.S_IFMT, os.S_IFLNK);

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

}

[buf, err] = os.getcwd();
Expand Down Expand Up @@ -298,3 +317,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