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(ext/node): Add fs.lutimes / fs.lutimesSync #23172

Merged
merged 10 commits into from
Jul 3, 2024

Conversation

nathanwhit
Copy link
Member

Part of #18218

  • Adds fs.lutimes and fs.lutimesSync to our node polyfills. To do this I added methods to the FileSystem trait ops to expose the functionality to JS.
  • Exports fs._toUnixTimestamp. Node exposes an internal util toUnixTimestamp from the fs module to be used by unit tests (so we need it for the unit test to pass unmodified). It's weird because it's only supposed to be used internally but it's still publicly accessible
  • Matches up error handling and timestamp handling for fs.futimes and fs.utimes with node
  • Enables the node_compat utimes test - this exercises futimes, lutimes, and utimes.

@nathanwhit
Copy link
Member Author

nathanwhit commented Apr 3, 2024

The node_compat test is failing on the windows runner due to a permission denied error when making (or maybe removing?) a temp file. I don't have a windows machine to debug this on, but it's odd that it only fails on the test debug job (and not release). It may be a bug in the tmpdir.js module, which we vendor from node, but it's hard to tell.

@nathanwhit
Copy link
Member Author

CI issue has been fixed, so this is now ready for review

@bartlomieju bartlomieju added this to the 1.45 milestone Jul 2, 2024
Copy link
Member

@littledivy littledivy left a comment

Choose a reason for hiding this comment

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

LGTM

@nathanwhit nathanwhit merged commit dadc606 into denoland:main Jul 3, 2024
17 checks passed
zebreus pushed a commit to zebreus/deno that referenced this pull request Jul 8, 2024
Part of denoland#18218


- Adds `fs.lutimes` and `fs.lutimesSync` to our node polyfills. To do
this I added methods to the `FileSystem` trait   ops to expose the
functionality to JS.
- Exports `fs._toUnixTimestamp`. Node exposes an internal util
`toUnixTimestamp` from the fs module to be used by unit tests (so we
need it for the unit test to pass unmodified). It's weird because it's
only supposed to be used internally but it's still publicly accessible
- Matches up error handling and timestamp handling for fs.futimes and
fs.utimes with node
- Enables the node_compat utimes test - this exercises futimes, lutimes,
and utimes.
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