Skip to content
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.

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

[ node ] Add missing FFI functions for node #3500

Merged
merged 5 commits into from
Mar 1, 2025

Conversation

dunhamsteve
Copy link
Collaborator

Description

The openDir FFI is implemented for the node backend, but it does not compile due to a reference to prim__getErrno. I've added prim__getErrno for node, following the pattern used for file errors.

In addition to this, I've implemented three System.Term FFI functions. As in the non-windows C backend, prim__setupTerm is a no-op. prim__getTermCols and prim__getTermLines return the number of columns and lines, respectively.

Finally, I put in a no-op for prim__flush on node. I didn't see a flush in the node api. This is used for stdout in the REPL and on the file handle in ide mode. The REPL in node appears to flush output characters automatically. I haven't ported IDE mode yet.

These all came up when building an Idris compiler that runs in javascript. If IDE mode and scheme eval are commented out, these changes are sufficient to build and run on JS.

@mattpolzin
Copy link
Collaborator

The C functions for getting errno and file errno are different so I don't imagine we can use the same NodeJS implementation for both.

@dunhamsteve
Copy link
Collaborator Author

dunhamsteve commented Feb 23, 2025

I can move it to a separate function to make the semantics match. On node the whole errno mechansim is faked, but the fileErrno in Idris does try to translate the error to a Idris-specific number and getErrno does not.

Note that in the RefC/Scheme backends prim__fileErrno and prim__getErrno are both based on the errno global. You may be thinking of the similarly named idris2_fileError, which passes a file handle to ferror().

int idris2_fileError(FILE *f) { return ferror(f); }
int idris2_fileErrno() {
switch (errno) {
case ENOENT:
return 2;
case EACCES:
return 3;
case EEXIST:
return 4;
default:
return (errno + 5);
}
}

@mattpolzin
Copy link
Collaborator

On node the whole errno mechansim is faked

Meaning that the result in Idris code won't be the same as if you were calling the same function depending on if you use the C or NodeJS backends? That's not great (and not documented in the Idris code that I can tell, though that's not on you to do in this PR).

You may be thinking of the similarly named idris2_fileError

Nope, I was calling out the fact that idris2_getErrno returns errno directly whereas idris2_fileErrno transforms it so the two are not drop-in equivalent in the C FFI.

- adds missing cases and fixes fallback values
- fix sign of getErrno
@dunhamsteve
Copy link
Collaborator Author

I had the sign wrong on the previous commit. It looks like the exception objects have a negative error number in the error field and errno is expected to be positive. The libuv documentation that they link to says "on Windows they are defined by libuv to arbitrary negative numbers".

I fixed the sign and I think we have parity for prim__errno for unix. I'm not sure what numbers windows returns in errno on the chez backend. It will be values like 4092 for EACCESS with this latest code. (I can run experiments in CI later, but I need to make dinner now.)

For prim__fileErrno, it looks like the current javascript support code only correctly handles Idris file err numbers 2 (ENOENT) and 4 (EEXIST) and returns bad values for fallback cases. The C code also handles 3 (EACCESS) and correctly maps the remaining errors above 4. (Codes 0 and 1 appear to be unused in C.) I've now updated the javascript code to get this correct (consulting the libuv source for the windows numbers), but it could use a test.

Which brings me to another question. Before this PR, if you run the base/system_directory test on node (manually trying to compile the file), you get:

% IDRIS2_CG=node ./run ~/.pack/bin/idris2 
Error: INTERNAL ERROR: No supported backend found in the definition of System.Errno.prim__getErrno. Supported backends: "node","javascript". Backends in definition: "C".

But tests/Main.idr says:

baseLibraryTests : IO TestPool
baseLibraryTests = testsInDir "base" "Base library" {requirements = [Chez, Node]}

Are these tests supposed to run on node?

@dunhamsteve
Copy link
Collaborator Author

Meaning that the result in Idris code won't be the same as if you were calling the same function depending on if you use the C or NodeJS backends?

So I said "faked" in that support doesn't see C errno and is making its own global that emulates errno. I didn't know how valid the numbers were at that point, but it appears that the number that it pulls off the exception matches C error codes. Windows is a bit weird there - it's possible that it's weird in scheme/C too. I'll have to check. (I don't yet know if those numbers are a windows thing or a node+windows thing.)

@dunhamsteve
Copy link
Collaborator Author

Yeah, for readFile and createDir on windows chez, errno is 0 on failure. Test was:

main : IO ()
main = do
  Left err <- readFile "xxxx" | Right _ => putStrLn "exists?"
  putStrLn "ENOENT is \{show !(getErrno)}"
  Left err <- createDir "build" | Right _ => putStrLn "can create?"
  putStrLn "EEXIST is \{show !(getErrno)}"

which gave:

idris2/error/xyzzy: FAILURE                                      02.812s
Expected:
ENOENT is 2
EEXIST is 17

Given:
ENOENT is 0
EEXIST is 0

@mattpolzin
Copy link
Collaborator

Which brings me to another question. Before this PR, if you run the base/system_directory test on node [...]
Are these tests supposed to run on node?

Ideally all of base would be tested on NodeJS, but since it has been an ongoing project to implement some of the primitives needed by base they are not run automatically (intentionally).

NodeJS is nevertheless required in order to run the base tests because some of the tests do explicitly test NodeJS, like this one: https://github.com/idris-lang/Idris2/blob/main//tests/base/system_time001/run#L5..L6

@mattpolzin
Copy link
Collaborator

Yeah, for readFile and createDir on windows chez, errno is 0 on failure

I'm tempted to be okay with that -- to feel that consistency between backends on the same system is more important than consistency between platforms. I wish Windows implemented a faithful errno, but some added code documentation in the Idris code to alert any downstream users of the functions to the need to handle the result differently depending on OS seems sufficient to me.

Thanks for not only adding to the NodeJS primitive surface area but also adding the missing cases that had sat as a TODO for a long time!

@mattpolzin
Copy link
Collaborator

Tangentially, I do think a reasonable test case could be written that expected different results for Windows vs. Unix, but I would merge this without that too.

@dunhamsteve
Copy link
Collaborator Author

dunhamsteve commented Mar 1, 2025

The 0 error numbers are not ideal, but I think a test that they're equal to 0 will simply verify that something is broken. I've added documentation to getErrno noting that it doesn't always work on windows. I don't know if it ever works.

I've removed my node029 test and added node to the system_directory test that it had been copied from.

And I've added a test that checks if file errors are getting translated correctly (in base for both chez and node). This test found a bug in my PR for windows errors in node (I needed -4048 EACCES instead of -4092 EPERM).

Copy link
Collaborator

@mattpolzin mattpolzin left a comment

Choose a reason for hiding this comment

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

Looks good to me

@mattpolzin mattpolzin merged commit 0422ee5 into idris-lang:main Mar 1, 2025
22 checks passed
@dunhamsteve dunhamsteve deleted the node-ffi branch March 1, 2025 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants