On Sun, 13 Oct 2024 15:38:49 GMT, Simon Tooke <[email protected]> wrote:
>> This PR changes the status of realpath() from a Posix-specific API to a
>> globally available API, i.e. adding it to the "Hotspot Porting API". Code
>> would refer to os::realpath() instead of os::Posix::realpath().
>>
>> This requires a Windows implementation of realpath(), using Windows
>> _fullpath(), and renaming os::Posix::realpath() to os::realpath().
>>
>> The main difference between POSIX and Windows behaviour is that POSIX
>> actually requires an existing accessible file, while Windows will happily
>> work with made-up filenames.
>>
>> Please note that guidelines for doing this appear in
>> src/hotspot/share/runtime/os.hpp
>
> Simon Tooke has updated the pull request incrementally with one additional
> commit since the last revision:
>
> clean up test code
Hi @stooke , I was away on vacation when you made the updates.
I'm afraid I still have some issues about expected behaviour on Windows with
regard to the tests.
test/hotspot/gtest/runtime/test_os.cpp line 413:
> 411:
> 412: TEST_VM(os, realpath) {
> 413: /* POSIX requires that the file exists, Windows doesn't */
I'm not sure what this comment means. I can't fully discern from
https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/fullpath-wfullpath?view=msvc-170
what Windows does if the path does not exist.
test/hotspot/gtest/runtime/test_os.cpp line 422:
> 420: errno = 0;
> 421: const char* returnedBuffer = os::realpath(nosuchpath, buffer,
> sizeof(nosuchpath) - 2);
> 422: /* Returns ENOENT on Linux, ENAMETOOLONG on Windows */
Suggestion:
/* Reports ENOENT on Linux, ENAMETOOLONG on Windows */
and similarly below.
test/hotspot/gtest/runtime/test_os.cpp line 425:
> 423: EXPECT_TRUE(returnedBuffer == nullptr);
> 424: #if defined(_WINDOWS)
> 425: EXPECT_TRUE(errno == ENAMETOOLONG);
Why is this the case? Our implementation does not set it and `_fullpath` makes
no mention of it.
-------------
PR Review: https://git.openjdk.org/jdk/pull/20683#pullrequestreview-2370878680
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1802144245
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1802139054
PR Review Comment: https://git.openjdk.org/jdk/pull/20683#discussion_r1802142390
