Skip to content

Conversation

@jameshu15869
Copy link
Contributor

This PR adds nanosecond support for file times in WasmFS, specifically when used for utime and stat. This PR was separated from #19561 (comment), reusing the same tests.

@tlively
Copy link
Member

tlively commented Jun 15, 2023

It looks like the diff still contains changes from #19561. If you change the target branch for this PR from main to jameshu15869:library-wasmfs-utime, that would fix the diff. Although it looks like that branch is in your fork, so this probably isn't possible, unfortunately. We'll have to wait until #19561 is merged to get a reasonable diff here.

@jameshu15869
Copy link
Contributor Author

jameshu15869 commented Jun 15, 2023

Ah I see. For this PR, I did git checkout -b on my old (combined) branch before manually undoing the nanosecond changes on #19561. Is there a better way to do this in the future?

@tlively
Copy link
Member

tlively commented Jun 15, 2023

One way would be to add a commit undoing the changes on the original PR, then fork a new branch off of it, then revert the undoing of the changes on the new branch so that the revert commit actually reapplies the changes. Then even if the diff for the PR that adds the changes includes everything, we can just look at the diff for that revert commit to see only the new changes.

return err;
}

// TODO: Set tv_nsec (nanoseconds) as well.
Copy link
Member

Choose a reason for hiding this comment

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

this TODO can be removed now

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Code lgtm but we have some failures on main it seems, that are showing up here and everywhere else. Work is ongoing to fix those but this will need to wait to land on that.

@sbc100
Copy link
Collaborator

sbc100 commented Jun 21, 2023

Rebasing or merging should fix the CI issues.

@kripken kripken merged commit d25f32e into emscripten-core:main Jun 21, 2023
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.

4 participants