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

[RDY] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` #1034

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Hinidu
Copy link
Contributor

Hinidu commented Aug 5, 2014

5 days ago libuv 0.11.27 was released. It includes uv_fs_mkdtemp from joyent/libuv#1368. This PR should finally close #812.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 5, 2014

Pretty awesome it's been mainlined already, good work @Hinidu!

Now, I guess the reason it's been failing are the dependencies which are still pretty old. Perhaps if you have your say in neovim/bot-ci#8 et al we can see this in master quicker ;).

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 5, 2014

Pretty awesome it's been mainlined already, good work @Hinidu!

It was surprisingly fast release :-)

Now, I guess the reason it's been failing are the dependencies which are still pretty old. Perhaps if you have your say in neovim/bot-ci#8 et al we can see this in master quicker ;).

Thanks, I were aware about that issue. I'll leave a comment on mentioned PR later.
It seems that I should split this PR to two: one for upgrading libuv which allows to rebuild deps for travis and other for os_mkdtemp changes.

@Hinidu Hinidu changed the title Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` [WIP] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` Aug 6, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 7, 2014

@Hinidu other than the necessary neovim/deps upgrade, is this RDY? If so I will update neovim/deps to libuv 0.11.27

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

@tarruda Thanks, but actually I want to do some changes in the code of this PR, so I can wait neovim/bot-ci#8 or whatever.

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 7, 2014

@Hinidu other than the necessary neovim/deps upgrade, is this RDY? If so I will update neovim/deps to libuv 0.11.27

It would be better if we split it up in two PRs (one updates libuv, the other implements the change), so that travis can verify that everything is ok.

For that we would need to:

  1. Upgrade to v0.11.27
  2. Perform the rebuild-deps changes (so travis gets v0.11.27)

Are you planning to push that through now @tarruda?

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 7, 2014

Are you planning to push that through now @tarruda?

Yes I'm updating to libuv 0.11.28 right now and will create a separate PR for it

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 7, 2014

Yes I'm updating to libuv 0.11.28 right now and will create a separate PR for it

I don't complain, go ahead!

@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 7, 2014

Yes I'm updating to libuv 0.11.28 right now and will create a separate PR for it

Sounds good, it will be nice when we're able to rebuild dependencies with ease. Either in this repo or in bot-ci.

@fwalch

This comment has been minimized.

Copy link
Member

fwalch commented Aug 7, 2014

@tarruda If all the dependencies are rebuilt, i.e. the updated luarocks version (from #1018) gets into neovim/deps before #1031 is fixed, all Travis builds will fail (make unittest won't work). The luarocks update (was because of #1013) could be reverted, though, because luarocks.org is now up again.
Just wanted to let you know, if you only update libuv there shouldn't be a problem.

Edit: wait.. I think you can ignore this comment.. if it were true this build using the auto-generated dependencies from neovim/bot-ci#8 should not have passed either?

Edit 2: Ugh, sorry for the confusion, forgot that this build had an additional commit with a quick hack to circumvent #1031. So I think things will fail if luarocks is updated.

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 7, 2014

Opened #1047 for the libuv update, and already pushed changes to neovim/deps

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 7, 2014

@fwalch I only rebuilt libuv, leaving luarocks untouched for now :)

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 7, 2014

Merged #1047, feel free to rebase

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 8, 2014

@tarruda thanks! Rebased, mark as RFC.

@Hinidu Hinidu changed the title [WIP] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` [RFC] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` Aug 8, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 8, 2014

LGTM

1 similar comment
@aktau

This comment has been minimized.

Copy link
Member

aktau commented Aug 8, 2014

LGTM

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 8, 2014

Thanks for reviews! I'll update :h tempfile in a separate commit (now it tells that function guarantees only 26 different results) and tag it RDY when I will have access to computer.

@Hinidu

This comment has been minimized.

Copy link
Contributor

Hinidu commented Aug 8, 2014

Updated documentation, if this update is ok then it can be merged. Mark as RDY.

@Hinidu Hinidu changed the title [RFC] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` [RDY] Implement `os_mkdtemp` on top of `uv_fs_mkdtemp` Aug 8, 2014

@tarruda

This comment has been minimized.

Copy link
Member

tarruda commented Aug 9, 2014

👍 merged, thanks

@tarruda tarruda closed this Aug 9, 2014

tarruda added a commit that referenced this pull request Aug 9, 2014

@Hinidu Hinidu deleted the Hinidu:uv_fs_mkdtemp branch Aug 13, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment