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

Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat; requires previous PRs #1635

Conversation

LeeTibbert
Copy link
Contributor

@LeeTibbert LeeTibbert commented Jun 23, 2019

Documentation:

  • The standard changelog entry is requested.

  • docs/lib/javalib.rst and docs/lib/posixlib.rst have each been edited
    to describe the changes of this PR and how to adapt existing code.

Testing:

  • Built and tested ("test-all") in debug mode using sbt 1.2.8 on
    X86_64 only . All tests pass.

…& posixlib sys/stat

* This PR fixes Issue scala-native#1610 "SN only supports seconds resolution
  for FileTime".  My thanks to @avdv for detecting & reporting this
  issue.

  Nanosecond resolutions are now supported in both javalib FileTime
  and posixlib sys/stat where the underlying operating and file systems
  support nanoseconds.

* **THIS CHANGE REQUIRES CHANGES TO EXISTING APPLICATIONS WHICH USE
  TIME FIELDS IN `struct stat`.**  The required changes are described
  in new edits to `docs/lib/posixlib` and `docs/lib/javalib`.

* Instant.scala was upgraded only to the point of allowing this PR.
  Extensive work, beyond the scope of this PR, is needed in `java.date.time`.

* A unit test was added to `java/nio/file/FilesSuite.scala` to
  set and read a nanosecond file time. This test was designed to work
  where nanosecond file times are supported and not fail where they
  are not.

* The test suite `java/time/InstantSuite.scala` was added to validate
  Instant.scala.

Documentation:

* The standard changelog entry is requested.

* `docs/lib/javalib.rst` and `docs/lib/posixlib.rst` have each been edited
to describe the changes of this PR and how to adapt existing code.

Testing:

* Built and tested ("test-all") in debug mode using sbt 1.2.8 on
    X86_64 only . All tests pass.
@avdv
Copy link
Contributor

avdv commented Nov 19, 2019

Hi.

Just to note, this does not work on macOS:

[error] /Users/travis/build/avdv/scalals/native/target/scala-2.11/native/lib/stat.c:44:30: error: no member named 'st_atim' in 'struct stat'

[error]     my_stat->st_atim = stat->st_atim;

[error]                        ~~~~  ^

[error] /Users/travis/build/avdv/scalals/native/target/scala-2.11/native/lib/stat.c:45:30: error: no member named 'st_mtim' in 'struct stat'

[error]     my_stat->st_mtim = stat->st_mtim;

[error]                        ~~~~  ^

[error] /Users/travis/build/avdv/scalals/native/target/scala-2.11/native/lib/stat.c:46:30: error: no member named 'st_ctim' in 'struct stat'

[error]     my_stat->st_ctim = stat->st_ctim;

[error]                        ~~~~  ^

[error] 3 errors generated.

[error] java.lang.RuntimeException: Failed to compile native library runtime code.

[error] 	at scala.sys.package$.error(package.scala:26)

[error] 	at scala.scalanative.build.LLVM$.$anonfun$compileNativelib$6(LLVM.scala:108)

[error] 	at scala.scalanative.build.LLVM$.$anonfun$compileNativelib$6$adapted(LLVM.scala:92)

[error] 	at scala.collection.parallel.mutable.ParArray$ParArrayIterator.foreach(ParArray.scala:142)

[error] 	at scala.collection.parallel.ParIterableLike$Foreach.leaf(ParIterableLike.scala:970)

[error] 	at scala.collection.parallel.Task.$anonfun$tryLeaf$1(Tasks.scala:49)

[error] 	at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.java:12)

[error] 	at scala.util.control.Breaks$$anon$1.catchBreak(Breaks.scala:63)

[error] 	at scala.collection.parallel.Task.tryLeaf(Tasks.scala:52)

[error] 	at scala.collection.parallel.Task.tryLeaf$(Tasks.scala:46)

[error] 	at scala.collection.parallel.ParIterableLike$Foreach.tryLeaf(ParIterableLike.scala:967)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.internal(Tasks.scala:166)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.internal$(Tasks.scala:153)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingForkJoinTasks$WrappedTask.internal(Tasks.scala:436)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.compute(Tasks.scala:146)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingTasks$WrappedTask.compute$(Tasks.scala:145)

[error] 	at scala.collection.parallel.AdaptiveWorkStealingForkJoinTasks$WrappedTask.compute(Tasks.scala:436)

[error] 	at java.base/java.util.concurrent.RecursiveAction.exec(RecursiveAction.java:189)

[error] 	at java.base/java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:290)

[error] 	at java.base/java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1603)

[error] 	at java.base/java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:177)

[error] (scalalsNative / Compile / nativeLink) Failed to compile native library runtime code.

@LeeTibbert
Copy link
Contributor Author

@avdv Thank you for reporting this. Sorry that it failed and took your time.

There is almost certainly a case of a #define (my guess: __USE_XOPEN2K8) being setup
in a linux environment and not being set up on Apple. Arrgh!

Unfortunately, I do not have ready access to an Apple system. Which Apple system are
you on?

(Sorry if I am too concrete below. I am trying to save you probably quite limited time.)

If you have time to aid with debugging, could you kindly execute and post (or create git, or
PM me on gitter) the output of:

clang -x c /dev/null -dM -E

I am looking to see if APPLE or such is defined (along with various 64 bit defines).

Then, if the Apple license allows could you post the contents of the file which defines
struct stat. The file is almost certainly called stat.h but finding the right one can
be somewhat tricky. On Ubuntu 19.10 Linux, the file is located in /usr/include/x86_64-linux-gnu/bits.
This is the file which gets included by the <sys/stat.h> aka /usr/include/x86_64-linux-gnu/sys
which is evident in the failing stat.c file.

I an looking to see why the 64 bit definitions are not picked up.

Thank you for your help.

Lee

@ekrich
Copy link
Member

ekrich commented Nov 25, 2019

@LeeTibbert This may not be the exact one used but looks very close to the one on my system.
https://github.com/apple/darwin-xnu/blob/master/bsd/sys/stat.h
If not, they could have it elsewhere in a different part of the tree.

@avdv
Copy link
Contributor

avdv commented Nov 25, 2019

I found another resource: https://opensource.apple.com/source/xnu/xnu-1456.1.26/bsd/sys/stat.h.auto.html

Also, according to https://sourceforge.net/p/predef/wiki/OperatingSystems/ there should be a preprocessor variable called __APPLE__:

Type Macro Description
Identification macintosh Mac OS 9
Identification Macintosh Mac OS 9
Identification __APPLE__ && __MACH__ Mac OS X Defined by GNU C and Intel C++

@avdv
Copy link
Contributor

avdv commented Nov 25, 2019

Oh BTW, I do not use a Mac, this was on Travis: https://travis-ci.com/avdv/scalals/jobs/258405832#L1684

@avdv
Copy link
Contributor

avdv commented Nov 25, 2019

I ran some tests. Found that on OSX there is a struct timespec:

struct timespec {
 __darwin_time_t tv_sec;

 long tv_nsec;
};

And the time members of the struct stat are of this type:

struct timespec st_atimespec;

struct timespec st_mtimespec;

struct timespec st_ctimespec;

The following code compiles for Linux and OSX:

#include <sys/stat.h>

long f() {
  struct stat s;

# ifndef __APPLE__
  return s.st_mtim.tv_nsec;
# else
  return s.st_mtimespec.tv_nsec;
# endif
}

See https://travis-ci.com/avdv/scalals/jobs/260362797 for the details.

@avdv
Copy link
Contributor

avdv commented Nov 26, 2019

The following patch fixes it for me 🎉

diff --git a/nativelib/src/main/resources/stat.c b/nativelib/src/main/resources/stat.c
index dd149fc9..3b4e3ac3 100644
--- a/nativelib/src/main/resources/stat.c
+++ b/nativelib/src/main/resources/stat.c
@@ -41,9 +41,15 @@ void scalanative_stat_init(struct stat *stat,
     my_stat->st_uid = stat->st_uid;
     my_stat->st_gid = stat->st_gid;
     my_stat->st_size = stat->st_size;
+#ifdef __APPLE__
+    my_stat->st_atim = stat->st_atimespec;
+    my_stat->st_mtim = stat->st_mtimespec;
+    my_stat->st_ctim = stat->st_ctimespec;
+#else
     my_stat->st_atim = stat->st_atim;
     my_stat->st_mtim = stat->st_mtim;
     my_stat->st_ctim = stat->st_ctim;
+#endif
     my_stat->st_blocks = stat->st_blocks;
     my_stat->st_blksize = stat->st_blksize;
     my_stat->st_nlink = stat->st_nlink;
                                                                                                                                                   

@ekrich
Copy link
Member

ekrich commented Nov 26, 2019

@avdv I couldn't see much choice but your solution - More modern POSIX has the correct implementation but it appears Apple is behind here.

@LeeTibbert
Copy link
Contributor Author

@avdv, @ekrich Thank you for the time & research. A few quick thoughts

  1. I like the #if FOO form just above rather than the #ifndef FOO on the experimental entry before that. I find that non-negated code is easier for "intellectually challenged persons in a hurry", like me.

  2. I propose the for __APPLE__ && __MACH__ in an earlier entry, rather than just __APPLE__.
    That gives us OSX/Darwin and is subject being broken by OSX+1 or such. I am somewhat
    reluctant to use the simple APPLE because someone will almost certainly try to compile
    in an untested environment (to wit, the existence of this issue 😀 because I did not
    test on OSX.)

Do either of you know if SN is supposed to be supporting Apple < OSX? If yes, I could see
if those version are supposed to support 64 bit timestamps. Or I/we can document a restriction.
Either way, knowledge puts us in a better position.

  1. Understanding that the code in question is C code and was offered as "proof of concept" exploratory code: I propose an experiment with explicit casts from the APPLE type to the
    receiver type. Agreed, there is, unless Apple & the universe are cruel, structural equivalence,
    but it would be nice, IMHO, to have somewhat clean types, making the cast explicit.
    Some compiler or environment in the future is almost certain to set a stricter type checking option.

Thoughts?

This is Thanksgiving week in the USA. If we converge on a propose solution, I will try to edit
the PR late Sunday or Monday. Does that meet required time frames.

@LeeTibbert
Copy link
Contributor Author

Thoughts after a quick firehose education on Apple, OSX, macos, timestamps, etc.
I feel like one of those geese raised for pate which have a tube stuck down their throat and
are force fed grain....

  1. Testing for APPLE and MACH seems to be the preferred idiom for detecting OSX/macos.

  2. OSX is now 18 or so years old, so we can probably explicitly fail if MACH is not defined. That is, I propose a nested test, first for __APPLE, then for MACH then see below. If MACH define APPLE_MACH or such. if not then give an #error "Unsupported OS mumble". Probably excessively
    defensive but probably none of us has time or patience to futz with this in a year or two.

  3. The older APPLE HFS+ file system used 32 bit timestamps (pre 10.mumble). macOS v 10.13
    (circa late 2017) introduced the APFS (Apple File System) has 64 bit timestamps. I suspect that
    testing for just APPLE and MACH is not good enough, earlier macOS versions
    will not define 64 bit fields. I suspect that a test
    is needed for at least the macos version which introduced APFS. There is supposed to be an
    Apple file availability.h which gives macos versions. My next step is to figure out when that
    was introduced and/or what to do if it is not available.

  4. At some point, I may give up and do a test for the size of the structure. If small, assume 32 bit
    timestamps. UGLY but may get the job done (beware padding).

I figure I have 39 years and 360+ days left to wander in this Wilderness....

@LeeTibbert
Copy link
Contributor Author

OK, A path forward becomes clearer. It appears that OSX has had availability.h since 2007. Blessings upon the foresighted developer(s). That file defines AVAILIBILITY once included.

So I can test for AVAILABILITY. If not defined (short branch), #error Can not determine OS version, else proceed with the 64 bit code posted by @avdv. That should be robust.

Thoughts? Sound like a plan?

@ekrich
Copy link
Member

ekrich commented Nov 26, 2019

@LeeTibbert I am not so sure we need such a robust mechanism. I'm thinking the __APPLE__ as suggested is good enough. In reality, old OS versions are a security hazard by nature so I'm not sure worrying about antiquity is worth it unless we get a really good use case. Apple hardware older than a certain number of years does not support the newer OS versions and then they drop security patches as well.

@LeeTibbert
Copy link
Contributor Author

@ekrich Agreed about not supporting old versions, for all the reasons you state & probably a few more.
I just want to test for & reject old versions so that I/we do not have to catch complaints later, avoiding both irritated customers & irritated devos.

With a simple _APPLE, someone, somewhere, somewhen, will complain about this code not
compiling on an Apple watch and they will be right.

I try to get my intended edit in late Sunday or so, so that you and @avdv can have something
concrete to review. I do not think it will be as ugly or as convoluted as you think but that
is what review is for.

@LeeTibbert LeeTibbert changed the title Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat Fix #1610: add nanosecond resolution in javalib FileTime & posixlib sys/stat; requires previous PRs May 16, 2020
@LeeTibbert
Copy link
Contributor Author

To Everything There Is a Season (Ecclesiastes 3)

The merge of PR #2087 brings the season of this PR to an end. The underlying idea is still good & useful
but the implementation has become difficult with the passage of time and evolution of direction.

The current direction is that Instant.scala is implemented in the optional non-core package
javalib-ext-dummies. Since core packages, such as anything in javalib, can not rely on
optional packages, a private, non-public Instant or equivalent would need to be implemented in javalib.
Such an effort exceeds the limited time I now have available for development.

There are Work in Progress PRs which depended upon the merge of this PR. If & when I can
create available time, I hope to wheel through them and see if the file/files of this PR upon which
they depend can be folded into the WIP PR.

@LeeTibbert LeeTibbert closed this Jan 8, 2021
i10416 added a commit to i10416/scala-native that referenced this pull request Dec 25, 2022
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 added a commit to i10416/scala-native that referenced this pull request Dec 25, 2022
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 added a commit to i10416/scala-native that referenced this pull request Dec 25, 2022
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 added a commit to i10416/scala-native that referenced this pull request Dec 25, 2022
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
WojciechMazur pushed a commit that referenced this pull request Jan 10, 2023
* use high resolution file stat fields

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

* fix: remove wrong type declaration

* fix: put scalanative timespec back

* update: use statOps to access fields

- 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

* minor fix: address code review

* add test and inline docs

* docs: mention breaking changes in next release note
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.

None yet

3 participants