fix boost::filesystem::path for WIN32 ('\' instead of '/') #971

Diapolo commented at 9:20 PM on March 21, 2012: none

This commit fixes the filesystem paths to be Windows-style on Windows.

Currently this was added for the database directory, the database log, the dst and src path in the backup wallet operation, server.cert, server.pem, bitcoin.conf and bitcoind.pid.

Example from the debug.log: old: dbenv.open strLogDir=D:\Bitcoin\Wallet/database strErrorFile=D:\Bitcoin\Wallet/db.log new: dbenv.open LogDir=D:\Bitcoin\Wallet\database ErrorFile=D:\Bitcoin\Wallet\db.log

Tested on my Win7 machine and works.

luke-jr commented at 9:22 PM on March 21, 2012: member

Bleh, I bet boost has a const for this or something? :/

laanwj commented at 9:26 PM on March 21, 2012: member

Is this really needed? Using / should be perfectly fine on win32. All the OS functions can cope with slashes both ways.

Diapolo commented at 9:38 PM on March 21, 2012: none

Will take a look here: http://www.boost.org/doc/libs/1_49_0/libs/filesystem/v3/doc/portability_guide.htm ... sorry if I'm a bit over-motivated to contribute, but at least it would be more aesthetic to have no mixed paths with \ and /.

laanwj commented at 9:43 PM on March 21, 2012: member

Yes maybe boost has something like Python path.join(), that'd be nicer than having to special-case it all over the place :)

Diapolo commented at 10:39 PM on March 21, 2012: none

pathxyz.make_preferred() is what I need :). POSIX: no effect. Windows: convert slashes to backslashes.

http://www.boost.org/doc/libs/1_47_0/libs/filesystem/v3/doc/reference.html#path-make_preferred

Is there interest in me redoing my commit?

This would not work for strLogDir and strErrorFile as these are strings.

Diapolo commented at 6:51 AM on March 22, 2012: none

Updated to use the Boost filesystem::path.make_preferred() function and modified the change for the db dir and db.log.

gavinandresen commented at 1:15 PM on March 22, 2012: contributor

NACK. I hate #ifdefs.

Diapolo commented at 2:42 PM on March 22, 2012: none

Updated to only use Boost filesystem::path and removed all of my former WIN32 #ifdefs!

laanwj commented at 2:51 PM on March 22, 2012: member

Much better. Code changes are ok w/ me now, this ofc needs testing on all concerned platforms.

Diapolo commented at 2:58 PM on March 22, 2012: none

Right, needs testing, but at least path.string().c_str() were already used in the code and are verified to work. I can say on Windows it looks good, Wallet Backup works and paths in the debug.log are now Windows-style.

Diapolo commented at 2:03 PM on March 23, 2012: none

Rebased to keep pull request compatible :).

TheBlueMatt commented at 10:42 PM on March 23, 2012: member

It seems to me this adds more function calls to more complicated functions to give os-specific path separators when, though an os may prefer one or another, it accepts both. So it seems like this just slows down and complicates non-performance-critical code for absolutely no reason?

Diapolo commented at 10:58 PM on March 23, 2012: none

On Linux or POSIX OSes make_preferred() does nothing, so no harm here. On Windows, I as a Windows user really hate Log-Files or whatever, which display a POSIX style file-path, even if it's supported. What would Linux users think, if there were backslashes in path-names ;)?

laanwj commented at 7:39 AM on March 24, 2012: member

Using OS-specific path separators is the right way; we've been "lucky" in that we only have to deal with two path separators, and one OS accepts both. Still it might be better not to rely on such (legacy) behavior.

And if we later want to show the paths in UI (for example, if we add the diagnostic dialog box) we also want them to be sanitized to the OS.

I don't think the performance argument holds, this is not inner-loop code and is only called once on initialization. I'm 100% sure the boost function is not that slow to make any noticeable difference here.

Diapolo commented at 2:14 PM on March 24, 2012: none

I'm with @laanwj, as performance is really a not existing problem with 4 more make_preferred() calls out of any thread and the main-loop.

TheBlueMatt commented at 3:50 PM on March 24, 2012: member

I said performance wasnt critical here, but its always nice to keep in the back of your mind when coding. The issue I have is that it complicates the code further for very little gain, though I suppose if it ends up confusing users when it gets printed to debug.log, I suppose it really doesnt matter either way.

gavinandresen commented at 3:19 PM on March 25, 2012: contributor

ACK for 0.7

Diapolo commented at 8:46 PM on March 26, 2012: none

Rebase to current master.

Diapolo commented at 1:08 PM on March 31, 2012: none

Last commit is for bitcoinrpc.cpp, which now uses make_preferred(), too. I fixed some small coding style glitches, to match the changes to be even to the ones in db.cpp.

The "boost/filesystem.hpp" header is already included before the #ifdef USE_SSL, so I removed that.

updated db.cpp to use make_preferred() 9c24588e73

updated bitcoinrpc.cpp to use make_preferred() and removed double inclusion of boost/filesystem.hpp 93fb7489a7

updated util.cpp to use make_preferred() 36949554ab

fixed small error in bitcoinrpc.cpp 42c63d3ad2

gavinandresen merged this on Apr 9, 2012

gavinandresen closed this on Apr 9, 2012

suprnurd referenced this in commit b68e119e4d on Dec 5, 2017

lateminer referenced this in commit bbeabc4d63 on Oct 30, 2019

DrahtBot locked this on Sep 8, 2021