Skip to content

Conversation

@MarijnS95
Copy link

@MarijnS95 MarijnS95 commented May 22, 2025

When (cross?) compiling this crate to Windows (in a Rust project) using Clang 16 or newer, we're seeing these declaration errors:

> cargo b --target x86_64-pc-windows-msvc
warning: [email protected]: /usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/metis-sys-0.3.1/vendor/GKlib/io.c(63,18): error: call to undeclared function 'read'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
warning: [email protected]:    63 |     if ((rsize = read(fd, buf, tsize)) == -1)
warning: [email protected]:       |                  ^
...
warning: [email protected]: /usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/metis-sys-0.3.1/vendor/GKlib/io.c(84,17): error: call to undeclared function 'write'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
warning: [email protected]:    84 |     if ((size = write(fd, buf, tsize)) == -1)
warning: [email protected]:       |                 ^

(LIHPC-Computational-Geometry/metis-rs#43 (comment))

Now it's yet unknown (we haven't checked) if older Clang had these declarations in a header, or didn't treat this warning as error yet, but the functions are POSIX which Windows isn't required to implement. Fortunately they provide it but with a deprecation warning and a recommended replacement of _read() (and _write()), but for this <io.h> needs to be included:

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/posix-read
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read

The same is true for getpid() that commit a8e7e25 ("port to vs2017") used to provide a win32/adapt.c/h workaround for, but this can instead be pulled from <process.h>.

This warning is already disabled by CMake using _CRT_SECURE_NO_DEPRECATE, but only for MSVC builds but not yet when targeting clang (s/MSVC/WIN32 to fix):

set(GKlib_COPTIONS "-DWIN32 -DMSC -D_CRT_SECURE_NO_DEPRECATE -DUSE_GKREGEX")

@karypis
Copy link
Contributor

karypis commented Jul 4, 2025

Unfortunately, I do not have access to a windows machine to test the above. Can other people verify that the above fix works across recent versions of windows?

@MarijnS95
Copy link
Author

Hey @karypis, if I remember correctly this also happened specifically while cross-compiling the mentioned Rust crate referencing this library from a Linux/MacOS machine to Windows using clang. I might be able to cook up the right cmake invocations for cross-compilation so that you could repro the same without having direct access to a Windows machine.

@MarijnS95
Copy link
Author

Actually, I figured, why not compile natively on Windows using clang which was causing this error. GitHub Actions can do this directly and after a few tries (because the CMake here is only set up for MSVC but not Win32), here is the resulting test and the CI failure demonstrating this error:

MarijnS95@6760c62
https://github.com/MarijnS95/GKlib/actions/runs/16081155217/job/45386033704#step:4:347

@MarijnS95
Copy link
Author

MarijnS95 commented Jul 4, 2025

Of course, applying the commit from this PR on top fixes the issue:

https://github.com/MarijnS95/GKlib/actions/runs/16081310139/job/45386384098

The deprecation warning is still there, maybe we should use _CRT_SECURE_NO_WARNINGS over _CRT_SECURE_NO_DEPRECATE if this is intended.
EDIT: That file is no longer referenced so none of these are defined, since develop was merged to master with b8b8bdc#r161486286.

Note that the MSVC build still fails. Once that's over, do you mind if I PR my other CMakeLists.txt changes together with a minimal CI setup to help test and validate these issues in the future?

…h Clang

When (cross?) compiling this crate to Windows (in a Rust project) using
Clang 16 or newer, we're seeing these declaration errors:

    > cargo b --target x86_64-pc-windows-msvc
    warning: [email protected]: /usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/metis-sys-0.3.1/vendor/GKlib/io.c(63,18): error: call to undeclared function 'read'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    warning: [email protected]:    63 |     if ((rsize = read(fd, buf, tsize)) == -1)
    warning: [email protected]:       |                  ^
    ...
    warning: [email protected]: /usr/local/cargo/registry/src/index.crates.io-1949cf8c6b5b557f/metis-sys-0.3.1/vendor/GKlib/io.c(84,17): error: call to undeclared function 'write'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
    warning: [email protected]:    84 |     if ((size = write(fd, buf, tsize)) == -1)
    warning: [email protected]:       |                 ^

(LIHPC-Computational-Geometry/metis-rs#43 (comment))

Now it's yet unknown (we haven't checked) if older Clang had these
declarations in a header, or didn't treat this warning as error yet,
but the functions are POSIX which Windows isn't required to implement.
Fortunately they provide it but with a deprecation warning and a
recommended replacement of `_read()` (and `_write()`), but for this
`<io.h>` needs to be included:

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/posix-read
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/read

The same is true for `getpid()` that commit a8e7e25 ("port to vs2017")
used to provide a `win32/adapt.c/h` workaround for, but this can instead
be pulled from `<process.h>` (equally with a deprecation warning, but
we should probably address these all in a consistent manner instead
of defining a workaround).  Let's take this as a starting point and go
from there.
@MarijnS95 MarijnS95 force-pushed the read-write-on-windows branch from 9cb7831 to 2230482 Compare July 4, 2025 21:58
@MarijnS95
Copy link
Author

Note that this PR implictly fixes yet another build issue caused by the recent merge of the develop branch to master, this time on native Windows + MSVC with a Visual Studio project:

https://github.com/MarijnS95/GKlib/actions/runs/16098906662/job/45425457255#step:3:444

If I'm not mistaken this is caused by this removal:

84658df#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL13

As I don't think adding a single header file to the sources makes the entire surrounding folder part of the include directories:

84658df#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aR66

@mattmatician
Copy link

Unfortunately, I do not have access to a windows machine to test the above. Can other people verify that the above fix works across recent versions of windows?

I can confirm this fixed my Windows build issue.
Thanks @MarijnS95

@MarijnS95
Copy link
Author

Thanks for testing @mattmatician. I'm still waiting for @karypis to take a look at #46 which will test this in CI instead to demonstrate compatibility directly in PRs rather than requiring many users to test on a variety of setups.

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