0.5.x: use higher resolution file stat fields #3049

i10416 · 2022-12-25T11:19:05Z

rel #1610
prepare for reworking #1635: nanosecond resolution in javalib FileTime and posixlib sys/stat

This commit introduces breaking changes in public api

This commit changes stat struct fields so that it can use high resolution file stat, which is available since linux kernel 2.5.48 and later.
For more detail,see linux man page https://linux.die.net/man/2/stat

i10416 · 2022-12-25T12:41:55Z

Why Linux / Test runtime (3.2.1, release-fast, immix) failed?

InflaterTest.inflaterCanInflateByteArraysWhenGivenBufferIsSmallerThanTotalOutput failed due to DataFormatException.

i10416 · 2022-12-25T12:53:05Z

may be relevant

#1918 (comment)
#1918 (comment)

prepare for reworking scala-native#1635: nanosecond resolution in javalib FileTime and posixlib sys/stat This commit introduces breaking changes in public api. This commit changes stat struct fields so that it can use high resolution file stat, which is available since linux kernel 2.5.48 and later. For more detail,see linux man page https://linux.die.net/man/2/stat

i10416 · 2022-12-25T14:41:28Z

error disappears after running test again...🤔

i10416 · 2022-12-25T14:54:02Z

anyway, all checks have passed.

LeeTibbert · 2022-12-26T17:56:44Z

@i10416 Thank you for this PR and for taking some time to extract some
value from the carcass of PR #1635.

I have read through this PR on a first pass. Could merge of this be put off
for a few days so that I can do a detailed review?

I do not want to make things so difficult that nothing ever gets done.
I suspect that it is possible to introduce this PR in a way which causes
less breakage to existing users.

This week is a holiday week where I live, so it will take me a few days
into the New Year in order to be able to do a detailed review, such
as to check if the test from the #1635 might be used or adapted.
Also, identifying change the changes to the 0.5.0 changelog and
to posixlib (and clib?) documentation need to happen in order
to give users a sporting chance.

LeeTibbert · 2022-12-26T18:01:42Z

posixlib/src/main/resources/scala-native/types.h

@@ -10,6 +10,7 @@ typedef unsigned int scalanative_uid_t;
 typedef unsigned int scalanative_gid_t;
 typedef long long scalanative_off_t;
 typedef long int scalanative_time_t;
+typedef struct timespec scalanative_timespec;


I believe this line is improper. POSIX 2018 (and many before, I believe) specify timespec as being defined
in time.h, not types.h ( probably time was well established before types.h was specified.)

Some else in this PR there is probably an import which
needs to change or happen.

Thank you for feedback! I will address this later.

Hmm, could you tell me why scalanative_timespec introduced here?(link below) If we use timespec from time.h, I don't think scalanative_timespec is needed. Do you mean I should create another time.h file and put scalanative_timespec in it?

https://github.com/scala-native/scala-native/pull/2713/files#diff-9db9f7da0930f16af30649420a8fd8857d1b9f1e07366491539a6d5ef0c0eeccR36

By the way, reading the comment in sys/stat.c, I guess src/main/resources/scala-native/types.h intentionally violates spec for portability.

// We don't use the "standard" types such as `dev_t` for instance // because these have different sizes on eg. Linux and OSX. We use the // smallest type that can hold all the possible values for the different // systems. struct scalanative_stat { scalanative_dev_t st_dev; /** Device ID of device containing file. */ scalanative_dev_t st_rdev; /** Device ID (if file is character or block special). */ scalanative_ino_t st_ino; /** File serial number. */

These types above were introduced by Martin Duhem at 2017. Is this statement "these have different sizes on eg. Linux and OSX" not the case for recent versions?

NOTE: I'm not in a hurry😉 Have a nice holiday!

i10416 You ask good questions. Let me finish my
highest level concern, so the PR can progress, and return to the trickery at play here.

Quick answer to 1st question. I'll get back to second later (tomorrow?)

Lines 92-102 check that Scala Natives scala timespec can be passed thru
to C with no "copy-in/copy-out" code.

// struct timespec _Static_assert(sizeof(struct scalanative_timespec) == sizeof(struct timespec), "Unexpected size: struct timespec"); _Static_assert(offsetof(struct scalanative_timespec, tv_sec) == offsetof(struct timespec, tv_sec), "offset mismatch: timespec.tv_sec"); // and so on.

From software engineering view, it is unfortunate that the C declaration of scalanative_timespec
is hand edited and hand synchronized with the corresponding Scala struct. The test would
be stronger if the former where generated from the latter. For now, the costly safeguards
are education & review. In someplaces there are "If you change .scala, also change .c" comments.

Thank you!
So,

there exist scalanative_timespec and other scalanative_* structs so that we can verify Scala struct is the same format as c struct.

From software engineering view, it is unfortunate that the C declaration of scalanative_timespec
is hand edited and hand synchronized with the corresponding Scala struct. The test would
be stronger if the former where generated from the latter. For now, the costly safeguards
are education & review. In someplaces there are "If you change .scala, also change .c" comments.

I guess you mean it is better to put typedef struct timespec scalanative_timespec; in time.h for future maintenance, though the original PR Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat; requires previous PRs #1635 puts it in types.h.

Am I right?

LeeTibbert · 2022-12-26T19:24:10Z

anyway, all checks have passed.

Heisenbugs (indeterminate, flaky) bugs are the worst.

I have not seen this failure before but will keep my eyes out for it.

i10416 · 2022-12-27T23:16:59Z

ref #3050

i10416 · 2022-12-29T06:26:53Z

posixlib/src/main/resources/scala-native/sys/stat.c

-    scalanative_time_t _st_atime; /** Time of last access. */
-    scalanative_time_t _st_mtime; /** Time of last data modification. */
-    scalanative_time_t _st_ctime; /** Time of last status change. */
+    scalanative_timespec st_atim; /** Time of last access. */


I do not want to make things so difficult that nothing ever gets done.
I suspect that it is possible to introduce this PR in a way which causes
less breakage to existing users.

Hmm, how can we introduce public api changes in a less breaking way?
Is there way better than deprecation warning?

SN 0.5.0 is advertised/envisioned as requiring a re-build from sources and possibly requiring source changes.
That gives us some degrees of freedom to do things
a 'right' way.

So;

This PR should be marked in base topic as "0.5.0"

There should be an entry in `docs/changelog/0.5.0.md"
briefly describing the required source change and
pointing to an entry in javalib.rst

There should be a section in `docs/lib/javalib.rst'
giving a fuller description and examples of the
required source change (i.e. change foo._8 to
foo.a_time to use seconds, foo.a_tim to use timespec).

Once the dust settles on this PR, I can either help you
set up the Sphinx environment on your system to build .rst (use "Makefile html" not 'scripts/makedoc") and .md files or do a followup PR for you to review.

stat.c scalanative_stat_init() is exactly the kind of
glue code I have been trying to expunge from Scala Native.
After a bunch of network code, I had hoped to get to this one before it changed.

I think this section of code is correct/OK and will try
to queue up a follow on PR to optimize (probably to
optimize for Linux passthru with appropriate _Static guards.)
stat is supposed to be fast (but must always be correct).

There should be an entry in `docs/changelog/0.5.0.md" briefly describing the required source change and pointing to an entry in javalib.rst

There should be a section in `docs/lib/javalib.rst' giving a fuller description and examples of the required source change

Though I changed java File internal, it doesn't leak posixlib implementation for users.

In terms of source change, I think you mean docs/lib/posixlib, don't you?

Of course, we should mention that java File metadata will support nanosecond resolution in the future(probably in 0.5.x?).

I added docs here
174e9df
4e2096f

- improve readability and reduce making a mistake to access wrong fields - prevent stat.scala misc changes(e.g. changing field order) from spreading around codebase

posixlib/src/main/scala/scala/scalanative/posix/sys/stat.scala

LeeTibbert · 2023-01-02T19:05:00Z

@i10416 Thank you for your understanding & patience over the holidays.

I am beginning to work my way through this PR. It may take me awhile. I think
you are going in the right direction with recent statOps changes, etc. I think we
can tune/edit those to handle both the old 32 bit and new 64 bit (on some machines)
fields. I have done something similar with sockets for sin_len (present on MacOS & FreeBSD
but not on Linux). I need to develop a concrete example.

Let me see if I can answer some of your direct questions first.

javalib/src/main/scala/java/io/File.scala

javalib/src/main/scala/java/nio/file/attribute/PosixFileAttributeViewImpl.scala

posixlib/src/main/resources/scala-native/sys/stat.c

LeeTibbert · 2023-01-02T21:11:54Z

Review status, 2022-01-02 21:00 UTC

Things are looking pretty good at this point. Especially if we posit a (soon) follow-on documentation PR
(posixlib.rst, changelog/0.5.0.md).

ISO C has no stat.h, so clib issues are not a concern. Considered & answered.

My next sprint I hope to address two areas:

32 bit issues

The fact that multiarch 32 bit CI is passing adds confidence.
Testing.

This is related to the 32 bit above and subsumes it. I may clone this PR
and create a WIP PR which adds some tests. Currently there is no
posixlib StatTest.scala.

The fact that the existing javalib FilesMumble tests compile and execute
in CI adds confidence. I would like to review those tests to see how much
confidence they add. Files.setLastModifiedTime() (and relatives, if any.
Life has been good to me and it has been a long time since I have been
in that code) might be sufficient.

There is a test in PR Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat; requires previous PRs #1635 which might be useful to carry forward,
both for this PR and its javalib follow-on. test("Files.setLastModifiedTime works with nanoseconds")
It does not complain about 0 nanoseconds (assume system does not support).
Enough time has passed that a "cut & paste" would probably not work, but
the idea might. More archeology might reveal more or better candidates.

Sorry, am losing daylight & must run and give my back-brain some time to
ponder/improve.

i10416 · 2023-01-03T14:00:30Z

The fact that the existing javalib FilesMumble tests compile and execute in CI adds confidence. I would like to review those tests to see how much confidence they add.

As I looked through javalib File tests, I believe they cover good amount of current stat.scala stuffs.
Anyway, double checking gives more confidence.

In terms of nanosecond file metadata APIs, this PR focuses on stat fields in stat.c and stat.scala, and I will create another PR that covers file metadata APIs so that I can keep changes introduced by this PR small.
Thus, I plan to support nanosecond resolution file metadata APIs and corresponding tests(e.g. test("Files.setLastModifiedTime works with nanoseconds")) in that PR. Is it okey?

(I will add some tests for st_atimespec and kinds in this PR.)

i10416 · 2023-01-03T14:20:08Z

~~Reading stat spec, I found myself forgot adding st_atimensec and the similar for non POSIX system~~

LeeTibbert · 2023-01-03T18:28:57Z

LGTM - Looks good to me This PR looks ready for general review & merge in a day or so.
That would give some time for Apple related statOps changes to make the train.

To make it easier for reviewers to make sure the proper boxes are ticked:

re: documentation

I believe this is the first in a series of PRs. To not complicate this PR, I would
like to add, over the next week, some small but necessary edits to docs/changelog/0.5.0.md
and docs/lib/javalib.rst.

re: testing

i10415 may possibly adding some tests to either this PR or a separate javalib PR.
I will be doing some sandbox work to see if I can remove the copy-in/copy-out of
struct stat. That will require either using tests from this PR, if added before merge,
or my writing some tests.

@i10415

Thank you for your patience with my not being able to review this over the holidays.
Also, thank you for working through and responding to a number of discussions
in this PR.

Nice to see nanosecond time fields progressing.

LeeTibbert · 2023-01-12T00:41:59Z

@i10416 You mentioned possibly doing some work to extend higher resolution stat fields to
javalib FileTime.

Now that this PR has merged, could I encourage you to work on FileTime, if you have the
interest and time available.

If I can help, even by staying out of the way, please let me know.

PR #1635 used Instant.scala in NativePosixFileAttributeView
javalib-ext-dummies/src/main/scala/java/time/Instant.scala
provides a number of calls used in that PR.
/javalib/src/main/scala/java/nio/file/attribute/FileTime.scala provides FileTime.toInstant().

Of course, since that PR was created, Windows support has been added to SN. So the
issue of if & how Windows support can be added or worked around (always zero?) .

i10416 · 2023-01-12T02:54:59Z

could I encourage you to work on FileTime, if you have the
interest and time available.

Yes, off course.
Thank you for reminder, especially about Windows concerns.
I'm busy for a week, but I'm willng to work on file time stuff.

i10416 force-pushed the refine-time-struct-to-support-higher-resolution branch 2 times, most recently from cf57399 to 99b2fbf Compare December 25, 2022 11:33

i10416 force-pushed the refine-time-struct-to-support-higher-resolution branch from 99b2fbf to 55c0b58 Compare December 25, 2022 13:32

i10416 changed the title ~~use high resolution file stat fields~~ use higher resolution file stat fields Dec 25, 2022

LeeTibbert reviewed Dec 26, 2022

View reviewed changes

LeeTibbert mentioned this pull request Dec 26, 2022

posixlib & clib time.scala need rationalization #3053

Open

i10416 added 2 commits December 28, 2022 09:13

fix: remove wrong type declaration

b38df8b

fix: put scalanative timespec back

1c22d84

i10416 commented Dec 29, 2022

View reviewed changes

update: use statOps to access fields

99aad5d

- improve readability and reduce making a mistake to access wrong fields - prevent stat.scala misc changes(e.g. changing field order) from spreading around codebase

i10416 commented Dec 29, 2022

View reviewed changes

posixlib/src/main/scala/scala/scalanative/posix/sys/stat.scala Show resolved Hide resolved

LeeTibbert reviewed Jan 2, 2023

View reviewed changes

javalib/src/main/scala/java/io/File.scala Outdated Show resolved Hide resolved

LeeTibbert reviewed Jan 2, 2023

View reviewed changes

javalib/src/main/scala/java/nio/file/attribute/PosixFileAttributeViewImpl.scala Show resolved Hide resolved

LeeTibbert reviewed Jan 2, 2023

View reviewed changes

posixlib/src/main/resources/scala-native/sys/stat.c Show resolved Hide resolved

LeeTibbert reviewed Jan 2, 2023

View reviewed changes

posixlib/src/main/resources/scala-native/sys/stat.c Outdated Show resolved Hide resolved

minor fix: address code review

7ec4eac

i10416 force-pushed the refine-time-struct-to-support-higher-resolution branch from a5811b8 to 7ec4eac Compare January 3, 2023 13:38

i10416 force-pushed the refine-time-struct-to-support-higher-resolution branch 2 times, most recently from d10d88e to 80b1dae Compare January 4, 2023 15:09

add test and inline docs

174e9df

i10416 force-pushed the refine-time-struct-to-support-higher-resolution branch from 80b1dae to 174e9df Compare January 4, 2023 15:11

docs: mention breaking changes in next release note

4e2096f

i10416 changed the title ~~use higher resolution file stat fields~~ 0.5.0: use higher resolution file stat fields Jan 7, 2023

i10416 changed the title ~~0.5.0: use higher resolution file stat fields~~ 0.5.x: use higher resolution file stat fields Jan 7, 2023

WojciechMazur approved these changes Jan 10, 2023

View reviewed changes

WojciechMazur merged commit 5f97e57 into scala-native:main Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

0.5.x: use higher resolution file stat fields #3049

0.5.x: use higher resolution file stat fields #3049

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022

LeeTibbert commented Dec 26, 2022

LeeTibbert Dec 26, 2022

i10416 Dec 27, 2022

i10416 Dec 28, 2022 •

edited

i10416 Dec 28, 2022

LeeTibbert Jan 2, 2023

LeeTibbert Jan 2, 2023

i10416 Jan 3, 2023 •

edited

LeeTibbert commented Dec 26, 2022

i10416 commented Dec 27, 2022

i10416 Dec 29, 2022

LeeTibbert Jan 2, 2023

LeeTibbert Jan 2, 2023

i10416 Jan 3, 2023

i10416 Jan 7, 2023

LeeTibbert commented Jan 2, 2023

LeeTibbert commented Jan 2, 2023 •

edited

i10416 commented Jan 3, 2023 •

edited

i10416 commented Jan 3, 2023 •

edited

LeeTibbert commented Jan 3, 2023

LeeTibbert commented Jan 12, 2023

i10416 commented Jan 12, 2023

0.5.x: use higher resolution file stat fields #3049

0.5.x: use higher resolution file stat fields #3049

Conversation

i10416 commented Dec 25, 2022 • edited

i10416 commented Dec 25, 2022 • edited

i10416 commented Dec 25, 2022 • edited

i10416 commented Dec 25, 2022 • edited

i10416 commented Dec 25, 2022

LeeTibbert commented Dec 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i10416 Dec 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

i10416 Jan 3, 2023 • edited

Choose a reason for hiding this comment

LeeTibbert commented Dec 26, 2022

i10416 commented Dec 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LeeTibbert commented Jan 2, 2023

LeeTibbert commented Jan 2, 2023 • edited

i10416 commented Jan 3, 2023 • edited

i10416 commented Jan 3, 2023 • edited

LeeTibbert commented Jan 3, 2023

LeeTibbert commented Jan 12, 2023

i10416 commented Jan 12, 2023

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 commented Dec 25, 2022 •

edited

i10416 Dec 28, 2022 •

edited

i10416 Jan 3, 2023 •

edited

LeeTibbert commented Jan 2, 2023 •

edited

i10416 commented Jan 3, 2023 •

edited

i10416 commented Jan 3, 2023 •

edited