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

[ ttc ] Compare modification time with nanosecond precision #3046

Merged
merged 5 commits into from
Aug 31, 2023

Conversation

AlgebraicWolf
Copy link
Contributor

Description

This PR changes TTC modification time comparison to use nanosecond-precision timestamps. It addresses the issue described in #3042, but, unlike the original PR, it does not break safety guarantees and does not risk missed rebuilds of source files. Since getting high-precision timestamps is platform-dependent and might not be supported everywhere, it reverts to current behaviour whenever there is no proper way to get nanosecond part of file's mtime.

This PR depends on #3044 to pass CI and avoid bootstrapping the compiler when upgrading.

@@ -441,15 +442,18 @@ TTC Nat where

||| Get a file's modified time. If it doesn't exist, return 0 (UNIX Epoch)
Copy link
Member

Choose a reason for hiding this comment

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

Can you please edit the comment to document what these Ints are?
Better yet: why not introduce a record type with meaningful field names?

Copy link
Contributor Author

@AlgebraicWolf AlgebraicWolf Aug 28, 2023

Choose a reason for hiding this comment

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

That sounds reasonable, I'll add a record for timestamps.
I guess another improvement I should make is adding a function that would get both seconds and nanoseconds of a timestamp and return a structure from C that would be turned into the newly introduced record. This would allow us to get both parts of timestamp in a single system call and potentially prevent a dangerous situation where time is modified between two system calls.

@AlgebraicWolf AlgebraicWolf force-pushed the nanosecond-mtime branch 6 times, most recently from faa9657 to 4ab4991 Compare August 30, 2023 10:01
@AlgebraicWolf
Copy link
Contributor Author

I've introduced a record as requested. Additionally, I've revisited the low-level interface to add capability to get both parts of timestamp and all time attributes of a file in a single syscall. This should eliminate the possibility of races when checking file's time. It looks like currently this PR ticks off two points in #2207.

I have also discovered that the current implementation of prim__fileModifiedTime on node backend is buggy -- even though the return type is Int, its result may turn out to be a floating point number at runtime. This has been fixed as well.

@gallais gallais merged commit af7ba2f into idris-lang:main Aug 31, 2023
20 checks passed
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.

2 participants