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

Preliminary native windows support, part 2: standard library and libgerbil #1291

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rujialiu
Copy link
Contributor

@rujialiu rujialiu commented Jan 8, 2025

Much more changes this time :)

Copy link

netlify bot commented Jan 8, 2025

👷 Deploy request for elastic-ritchie-8f47f9 pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 60b339a

@vyzo vyzo requested a review from fare January 8, 2025 11:15
@vyzo
Copy link
Collaborator

vyzo commented Jan 8, 2025

@fare first pass again?

@rujialiu
Copy link
Contributor Author

rujialiu commented Jan 9, 2025

General notes

  1. We need win32ports's sys_time_h (https://github.com/win32ports/sys_time_h) and unistd_h (https://github.com/win32ports/unistd_h). That means I need to specify an external include path (in theory msvc can have "default include path" but most of the time we want to avoid it). Currently many codes are compiled even without using the default cc options form environment, so I had to add non-posix-extra-gsc-options to a few places. I didn't use cond-expand because I believe "respect default cc-options from environment variables" is also a correct dehavior for posix systems.
  2. There are quite a bit of code duplications but I decided to keep them because it'll make making platform-specific changes easier (and with more confidence)

Copy link
Collaborator

@fare fare left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few minor changes.

Apologies for the delay

src/std/make.ss Show resolved Hide resolved
@@ -40,6 +40,13 @@

(begin-ffi (_pipe make_pipe_ptr pipe_ptr_ref)
(c-declare "#include <unistd.h>")
(c-declare "
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't we be returning -1 for failure?

#define LOCK_NB 3
int flock(int fd, int op) {
return 0;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return -1 for error

(c-declare "
#ifdef _WINDOWS
int fsync(int fd) {
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return -1 for error

(c-declare "
#ifdef _WINDOWS
int fcntl (int __fd, int __cmd, ...) {
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return -1 for error

@@ -565,6 +582,9 @@ int ffi_socket_sendmsg (int fd, ___SCMOBJ name, ___SCMOBJ io, ___SCMOBJ ctl, int
msg.msg_flags = 0;

return sendmsg (fd, &msg, flags);
#else
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return -1 for error

(c-declare "
#ifdef _WINDOWS
int kill(int pid, int signo) {
return 0;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return -1 for error

@fare
Copy link
Collaborator

fare commented Jan 21, 2025

OK for the name, I guess.

Beware about the return values, though. I left comments on the patch at ef77b8c

@rujialiu
Copy link
Contributor Author

I've corrected the return values (including a few you didn't mention) in the latest commit. Did you find anything missing?

Copy link
Collaborator

@fare fare left a comment

Choose a reason for hiding this comment

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

Almost there.

(gxc: "os/fcntl" ,@(include-gambit-sharp))
(gxc: "os/flock" ,@(include-gambit-sharp))
(gxc: "os/pipe" ,@(include-gambit-sharp))
(gxc: "os/fd" ,@(include-gambit-sharp))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't introduce spurious whitespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. I was trying to make it prettier and easier to compare them. I'll remove it

src/std/make.ss Show resolved Hide resolved
@@ -624,6 +645,9 @@ int ffi_socket_recvmsg (int fd, ___SCMOBJ name, int *rname, ___SCMOBJ io, ___SCM
*rflags = msg.msg_flags;

return r;
#else
return -1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should also errno = EINVAL; or ERROR_NOT_SUPPORTED whenever we return -1; like that so that the error message will be appropriate (unless there's a better E code under Windows)—if it's ERROR_NOT_SUPPORTED we need it defined on the Scheme side, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a systems programmer (I mostly work on algorithms, graphics, gamedev etc) so finding the "right thing to do" is difficult for me... But since I'll summarize current supported/unsupported things in the upcoming PR (documents and scripts), can we leave these problems for the future?

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.

3 participants