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

Fixes and improvements for Windows platform code #5020

Merged
merged 3 commits into from Oct 20, 2016

Conversation

Projects
None yet
8 participants
@vit-stepanovs
Contributor

vit-stepanovs commented Oct 18, 2016

  • Changes for more efficient work with Windows file system (based on our previous contribution to RocksDb project)
  • Implementation for ReadOnlyMemoryRegion on Windows.
  • Fixes for WindowsFileSystem::GetChildren(), WindowsFileSystem::RenameFile(), port.cc Hostname().
  • Other minor fixes.
@tensorflow-jenkins

This comment has been minimized.

Show comment
Hide comment
@tensorflow-jenkins

tensorflow-jenkins Oct 18, 2016

Collaborator

Can one of the admins verify this patch?

Collaborator

tensorflow-jenkins commented Oct 18, 2016

Can one of the admins verify this patch?

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Oct 18, 2016

@vit-stepanovs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrry, @meteorcloudy and @keveman to be potential reviewers.

mention-bot commented Oct 18, 2016

@vit-stepanovs, thanks for your PR! By analyzing the history of the files in this pull request, we identified @mrry, @meteorcloudy and @keveman to be potential reviewers.

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 18, 2016

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

googlebot commented Oct 18, 2016

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@googlebot googlebot added the cla: no label Oct 18, 2016

@mrry mrry self-assigned this Oct 18, 2016

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

Looks like the CLA bot might need some time to propagate (assuming your account is covered by a corporate CLA?). Can you try checking https://cla.developers.google.com/clas to see if the system has picked up your CLA yet?

Looking forward to reviewing this, thanks!

Contributor

mrry commented Oct 18, 2016

Looks like the CLA bot might need some time to propagate (assuming your account is covered by a corporate CLA?). Can you try checking https://cla.developers.google.com/clas to see if the system has picked up your CLA yet?

Looking forward to reviewing this, thanks!

@vit-stepanovs

This comment has been minimized.

Show comment
Hide comment
@vit-stepanovs

vit-stepanovs Oct 18, 2016

Contributor

Hi Derek,

I indeed don’t see the CLA via the below link. We have signed it ~ 3 hours ago, so maybe indeed it needs more time to propagate. I will check again in a few hours.

Thank you,
Vit

From: Derek Murray [mailto:notifications@github.com]
Sent: Monday, October 17, 2016 5:48 PM
To: tensorflow/tensorflow tensorflow@noreply.github.com
Cc: Vit Stepanovs vistepan@microsoft.com; Mention mention@noreply.github.com
Subject: Re: [tensorflow/tensorflow] Fixes and improvements for Windows platform code (#5020)

Looks like the CLA bot might need some time to propagate (assuming your account is covered by a corporate CLA?). Can you try checking https://cla.developers.google.com/clashttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcla.developers.google.com%2Fclas&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=I4%2FMPrbq%2B3%2B4ZsAiezgjNAalIuiz%2FODJNUGYc8TLDA0%3D&reserved=0 to see if the system has picked up your CLA yet?

Looking forward to reviewing this, thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftensorflow%2Ftensorflow%2Fpull%2F5020%23issuecomment-254375594&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=EgdVP2S9gJkD6kaAbZieehDTJUGVLmIXhaQ0ffl3tuI%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAVvkk0eOelctjQhZed--n0mAvnLfQH61ks5q1BckgaJpZM4KZPaG&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=xQKWEjjyTT7L6hBRIi%2F7wRlwhRHU%2BLA3o79l2cM8WhI%3D&reserved=0.

Contributor

vit-stepanovs commented Oct 18, 2016

Hi Derek,

I indeed don’t see the CLA via the below link. We have signed it ~ 3 hours ago, so maybe indeed it needs more time to propagate. I will check again in a few hours.

Thank you,
Vit

From: Derek Murray [mailto:notifications@github.com]
Sent: Monday, October 17, 2016 5:48 PM
To: tensorflow/tensorflow tensorflow@noreply.github.com
Cc: Vit Stepanovs vistepan@microsoft.com; Mention mention@noreply.github.com
Subject: Re: [tensorflow/tensorflow] Fixes and improvements for Windows platform code (#5020)

Looks like the CLA bot might need some time to propagate (assuming your account is covered by a corporate CLA?). Can you try checking https://cla.developers.google.com/clashttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcla.developers.google.com%2Fclas&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=I4%2FMPrbq%2B3%2B4ZsAiezgjNAalIuiz%2FODJNUGYc8TLDA0%3D&reserved=0 to see if the system has picked up your CLA yet?

Looking forward to reviewing this, thanks!


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ftensorflow%2Ftensorflow%2Fpull%2F5020%23issuecomment-254375594&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=EgdVP2S9gJkD6kaAbZieehDTJUGVLmIXhaQ0ffl3tuI%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAVvkk0eOelctjQhZed--n0mAvnLfQH61ks5q1BckgaJpZM4KZPaG&data=01%7C01%7Cvistepan%40microsoft.com%7C758720f6aef74218373f08d3f6f0597d%7C72f988bf86f141af91ab2d7cd011db47%7C1&sdata=xQKWEjjyTT7L6hBRIi%2F7wRlwhRHU%2BLA3o79l2cM8WhI%3D&reserved=0.

@vit-stepanovs

This comment has been minimized.

Show comment
Hide comment
@vit-stepanovs

vit-stepanovs Oct 18, 2016

Contributor

I signed it!

Contributor

vit-stepanovs commented Oct 18, 2016

I signed it!

@martinwicke

This comment has been minimized.

Show comment
Hide comment
@martinwicke

martinwicke Oct 18, 2016

Member

@willnorris, can we check this CLA manually? Can I check a CLA manually?

Member

martinwicke commented Oct 18, 2016

@willnorris, can we check this CLA manually? Can I check a CLA manually?

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@willnorris I verified that the corporate CLA is in place, but is there anything else that Vit needs to do before @googlebot will recognize his account?

Contributor

mrry commented Oct 18, 2016

@willnorris I verified that the corporate CLA is in place, but is there anything else that Vit needs to do before @googlebot will recognize his account?

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 18, 2016

Contributor

@willnorris, FYI #5051, #5052, and #5053 seem to have the same issue.

Contributor

mrry commented Oct 18, 2016

@willnorris, FYI #5051, #5052, and #5053 seem to have the same issue.

@willnorris

This comment has been minimized.

Show comment
Hide comment
@willnorris

willnorris Oct 18, 2016

(moving CLA conversation to email)

willnorris commented Oct 18, 2016

(moving CLA conversation to email)

@vit-stepanovs

This comment has been minimized.

Show comment
Hide comment
@vit-stepanovs

vit-stepanovs Oct 19, 2016

Contributor

I signed it!

Contributor

vit-stepanovs commented Oct 19, 2016

I signed it!

@googlebot

This comment has been minimized.

Show comment
Hide comment
@googlebot

googlebot Oct 19, 2016

CLAs look good, thanks!

googlebot commented Oct 19, 2016

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Oct 19, 2016

@mrry

These changes look great! Just a couple of points of clarification and a nit, and we should be good to merge.

Show outdated Hide outdated tensorflow/core/platform/windows/env.cc Outdated
}
return s;
UniqueCloseHandlePtr file_guard(hfile, CloseHandleFunc);

This comment has been minimized.

@mrry

mrry Oct 19, 2016

Contributor

Does the UniqueCloseHandlePtr serve a purpose here? (I would expect it to be used when there is the possibility of returning early, due to an error, but it looks like L272 can't fail, so we are just creating and destroying the guard for no purpose. Perhaps I'm missing something.)

@mrry

mrry Oct 19, 2016

Contributor

Does the UniqueCloseHandlePtr serve a purpose here? (I would expect it to be used when there is the possibility of returning early, due to an error, but it looks like L272 can't fail, so we are just creating and destroying the guard for no purpose. Perhaps I'm missing something.)

This comment has been minimized.

@vit-stepanovs

vit-stepanovs Oct 19, 2016

Contributor

You are right, UniqueCloseHandlePtr is not needed in this case - it was a left-over from refactoring. Removed.

@vit-stepanovs

vit-stepanovs Oct 19, 2016

Contributor

You are right, UniqueCloseHandlePtr is not needed in this case - it was a left-over from refactoring. Removed.

Show outdated Hide outdated tensorflow/core/platform/windows/windows_file_system.cc Outdated

NORTHAMERICA\vistepan added some commits Oct 19, 2016

@mrry

mrry approved these changes Oct 19, 2016

LGTM! Thanks for the changes :). I'll just run the tests and we should be good to merge.

@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 19, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 19, 2016

@tensorflow-jenkins test this please.

1 similar comment
@mrry

This comment has been minimized.

Show comment
Hide comment
@mrry

mrry Oct 20, 2016

Contributor

@tensorflow-jenkins test this please.

Contributor

mrry commented Oct 20, 2016

@tensorflow-jenkins test this please.

@mrry mrry merged commit 46110c5 into tensorflow:master Oct 20, 2016

10 checks passed

Android Demo App SUCCESS
Details
Linux CPU Tests SUCCESS
Details
Linux CPU Tests (Python 3) SUCCESS
Details
Linux CPU Tests CMAKE SUCCESS
Details
Linux GPU SUCCESS
Details
MacOS CPU Tests SUCCESS
Details
Sanity Checks SUCCESS
Details
Windows Cmake Tests SUCCESS
Details
ci.tensorflow.org SUCCESS
Details
cla/google All necessary CLAs are signed

@vit-stepanovs vit-stepanovs deleted the vit-stepanovs:windows-platform branch Oct 20, 2016

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