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

Replace unistd functions with cross platform counterparts #3300

Merged
merged 1 commit into from
Nov 10, 2015
Merged

Replace unistd functions with cross platform counterparts #3300

merged 1 commit into from
Nov 10, 2015

Conversation

eelstork
Copy link
Contributor

@eelstork eelstork commented Nov 7, 2015

Via unistd.h, caffe uses a handful of functions not readily available on non-Unix systems; boost is cross platform and affords more concise, more readable implementations (notably, with reference to MakeTempFilename and MakeTempDir).
To support this, CMake and Make builds are updated to link against boost_filesystem.

@eelstork
Copy link
Contributor Author

eelstork commented Nov 9, 2015

Thanks to @flx42 for suggesting a truly cross platform temp dir.

@ronghanghu
Copy link
Member

LGTM :) Replacing unistd.h with boost should make it easier to build caffe across platforms for community windows ports.

@flx42
Copy link
Contributor

flx42 commented Nov 9, 2015

You have another place where you are using "/tmp", I guess you could also remove it and use boost too.

@eelstork
Copy link
Contributor Author

@ronghanghu thanks for looking at this.
@flx42 fixing.

@ronghanghu
Copy link
Member

I plan to merge this PR, if other people don't object.

@shelhamer
Copy link
Member

👍

@bhack
Copy link
Contributor

bhack commented Nov 10, 2015

Are we not ready for tmpfile et al.?

ronghanghu added a commit that referenced this pull request Nov 10, 2015
Replace unistd functions with cross platform counterparts
@ronghanghu ronghanghu merged commit e9930da into BVLC:master Nov 10, 2015
@eelstork
Copy link
Contributor Author

@bhack I appreciate difficulties running Boost on iOS and Android. In the meantime I'm not sure that C++11 is a silver bullet for cross-platform support. With a little more refactoring we'll be able to handle this.

@futurely
Copy link

Did this solve the problem of potential race condition #1063?

@futurely
Copy link

It seems not. #3324

@ronghanghu
Copy link
Member

Oh, I didn't notice the race condition in #1063. My bad. This is probably causing directory creation to fail in #3324?

It seems that both versions (old and new) have potential race conditions, so it is a bit unclear to me why #3324 starts to appear.

@futurely
Copy link

The easiest solution is to add a macro USE_BOOST_FILESYSTEM and keep both versions.

@eelstork
Copy link
Contributor Author

@futurely I think we are going to do this the hard way.
@ronghanghu if I understand correctly #3324 isn't a regression caused by this PR, but we're seeing significantly more occurrences since merging this?

@ronghanghu
Copy link
Member

I'm not sure if there is more occurrence of #3324 after this PR. Seems tricky to do a perfect fix to the race here. At this point the function is only used in test cases so I guess the race condition shouldn't affect too much.

@eelstork
Copy link
Contributor Author

@ronghanghu I've seen a couple more. I think it's worth fixing.

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

Successfully merging this pull request may close these issues.

6 participants