utils and libraries: Replace deprecated Boost Filesystem functions #15880

hebasto commented at 6:26 PM on April 23, 2019: member

Boost Filesystem basename() and extension() functions are deprecated since v1.36.0.

See more: https://lists.boost.org/Archives/boost/2010/01/160905.php

Also this PR prevents further use of deprecated Boost Filesystem functions. Ref: https://www.boost.org/doc/libs/1_64_0/libs/filesystem/doc/index.htm#Coding-guidelines

Note: On my Linux system Boost 1.65.1 header /usr/include/boost/filesystem/convenience.hpp contains:

# ifndef BOOST_FILESYSTEM_NO_DEPRECATED

    inline std::string extension(const path & p)
    {
      return p.extension().string();
    }

    inline std::string basename(const path & p)
    {
      return p.stem().string();
    }

    inline path change_extension( const path & p, const path & new_extension )
    { 
      path new_p( p );
      new_p.replace_extension( new_extension );
      return new_p;
    }

# endif

UPDATE: Also removed unused code as noted by ryanofsky.

DrahtBot added the label UTXO Db and Indexes on Apr 23, 2019

DrahtBot added the label Wallet on Apr 23, 2019

in src/wallet/db.cpp:411 in e5ce5a1443 outdated

 407 | @@ -408,8 +408,8 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er
 408 |      LogPrintf("Using wallet %s\n", file_path.string());
 409 |  
 410 |      // Wallet file must be a plain filename without a directory
 411 | -    if (walletFile != fs::basename(walletFile) + fs::extension(walletFile))
 412 | -    {
 413 | +    fs::path wallet_file_path = fs::path(walletFile);

Empact commented at 10:46 PM on April 28, 2019:

nit: could construct directly

hebasto commented at 7:10 AM on April 29, 2019:

@Empact Thank you for your review. I'm not sure if I understand your comment correctly. Could you elaborate, please?

Empact commented at 8:36 AM on April 29, 2019:

-fs::path wallet_file_path = fs::path(walletFile);
+fs::path wallet_file_path(walletFile);

hebasto commented at 8:56 AM on April 29, 2019:

Thank you! Fixed.

hebasto force-pushed on Apr 29, 2019

in src/wallet/db.cpp:412 in 8823a11988 outdated

 407 | @@ -408,8 +408,8 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er
 408 |      LogPrintf("Using wallet %s\n", file_path.string());
 409 |  
 410 |      // Wallet file must be a plain filename without a directory
 411 | -    if (walletFile != fs::basename(walletFile) + fs::extension(walletFile))
 412 | -    {
 413 | +    fs::path wallet_file_path(walletFile);
 414 | +    if (walletFile != wallet_file_path.stem().string() + wallet_file_path.extension().string()) {

ryanofsky commented at 2:25 PM on April 29, 2019:

Should just drop this check. The way SplitWalletPath is defined this can never be false:

https://github.com/bitcoin/bitcoin/blob/8da1aa471eb9f062c99c3134d6151961619e9da5/src/wallet/db.cpp#L66

Also, the way stem() and extension() are defined you could replace them with a call to filename()

if (walletFile != wallet_file_path.filename().string()) {

https://www.boost.org/doc/libs/1_69_0/libs/filesystem/doc/reference.html#path-decomposition

ryanofsky approved

ryanofsky commented at 2:36 PM on April 29, 2019: member

utACK 8823a11988d0d4824a44eb2f34e37632e7a0f09a

hebasto force-pushed on Apr 29, 2019

hebasto commented at 6:53 PM on April 29, 2019: member

@ryanofsky Thank you for your review. Your comment has been addressed. Would you mind re-reviewing?

ryanofsky approved

ryanofsky commented at 7:08 PM on April 29, 2019: member

utACK 276a67490f795429c1af844966ad7960908576c0. Only change is replacing impossible to reach condition with an assert.

in src/wallet/db.cpp:411 in 276a67490f outdated

 407 | @@ -408,11 +408,7 @@ bool BerkeleyBatch::VerifyEnvironment(const fs::path& file_path, std::string& er
 408 |      LogPrintf("Using wallet %s\n", file_path.string());
 409 |  
 410 |      // Wallet file must be a plain filename without a directory
 411 | -    if (walletFile != fs::basename(walletFile) + fs::extension(walletFile))
 412 | -    {
 413 | -        errorStr = strprintf(_("Wallet %s resides outside wallet directory %s"), walletFile, walletDir.string());
 414 | -        return false;
 415 | -    }
 416 | +    assert(walletFile == fs::path(walletFile).filename().string());

promag commented at 9:18 PM on April 29, 2019:

Suggestion, either drop the assertion (GetWalletEnv always returns a plain filename) or make it complete because:

std::cout << path("/").filename();            // outputs "/"
std::cout << path(".").filename();            // outputs "."
std::cout << path("..").filename();           // outputs ".."

so for these cases the assertion passes.

promag commented at 9:22 PM on April 29, 2019: member

utACK 276a674.

Shouldn't 3edc71674ce79d81f4c5da94663787899f123719 be after 276a67490f795429c1af844966ad7960908576c0?

Remove dead code for walletFile check

SplitWalletPath() garanties the walletFile is a plain filename without a
directory.

4f65af97b4

hebasto force-pushed on Apr 29, 2019

hebasto commented at 9:58 PM on April 29, 2019: member

@promag thank you for your review.

Your comments have been addressed:

the assertion has been dropped
commits have been rearranged

promag commented at 10:45 PM on April 29, 2019: member

utACK 8d0add4.

Empact commented at 6:35 AM on April 30, 2019: member

utACK https://github.com/bitcoin/bitcoin/pull/15880/commits/8d0add4ea92b148e5ab8295e9fa9b6776db38c18 could squash the latter two

The test was introduced in #7691 fyi, seems fine to remove.

Replace deprecated Boost Filesystem function

Boost Filesystem basename() function is deprecated since v1.36.0.
Also, defining BOOST_FILESYSTEM_NO_DEPRECATED before including
filesystem headers is strongly recommended. This prevents inadvertent
use of old features, particularly legacy function names, that have been
replaced and are going to go away in the future.

a0a222eec0

hebasto force-pushed on Apr 30, 2019