Replace boost/fc filesystem #1011

huangminghuang · 2023-04-11T21:10:45Z

This PR

replaces all usage of boost and fc filesystem with std::filesystem,
creates a new class temp_cfile to replace the usages of boost::filesystem::unique_path which doesn't exists in std::filesystem due to safety concern,

Includes
AntelopeIO/appbase#15
AntelopeIO/appbase#16
AntelopeIO/appbase#17
AntelopeIO/appbase#18
AntelopeIO/appbase#19
AntelopeIO/appbase#20
AntelopeIO/appbase#21
AntelopeIO/chainbase#5
AntelopeIO/chainbase#6
AntelopeIO/chainbase#7
AntelopeIO/chainbase#8
AntelopeIO/chainbase#9

Resolves #620

spoonincode · 2023-04-11T21:33:07Z

This change will require gcc 9.1+ or clang 9+ (technically libstdc++ & libc++). Please increase the minimum version in the root cmake,

leap/CMakeLists.txt

Lines 38 to 47 in 69c4e00

    
           # http://stackoverflow.com/a/18369825 
        
           if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU") 
        
               if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 8.0) 
        
                   message(FATAL_ERROR "GCC version must be at least 8.0!") 
        
               endif() 
        
           elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "Clang" OR "${CMAKE_CXX_COMPILER_ID}" STREQUAL "AppleClang") 
        
               if (CMAKE_CXX_COMPILER_VERSION VERSION_LESS 5.0) 
        
                   message(FATAL_ERROR "Clang version must be at least 5.0!") 
        
               endif() 
        
           endif()

(I think it's okay to remove AppleClang completely from this check; what's there isn't correct anyways)

spoonincode · 2023-04-11T21:46:54Z

Can you test if it's possible to remove this workaround now

leap/.github/workflows/build.yaml

Lines 134 to 136 in 69c4e00

    
                     # jammy's boost 1.74 can stumble on an EXDEV via copy_file_range() it doesn't have a fallback for; re-eval once moving to std::filesystem 
        
                     export TMPDIR="$PWD/tmp" 
        
                     mkdir -p $TMPDIR

spoonincode · 2023-04-12T18:00:28Z

There is at least one remaining boost::filesystem here

leap/plugins/trace_api_plugin/include/eosio/trace_api/store_provider.hpp

Line 8 in 6726ca6

#include <boost/filesystem.hpp>

leap/plugins/trace_api_plugin/include/eosio/trace_api/store_provider.hpp

Line 16 in 6726ca6

using namespace boost::filesystem;

This causes a compile error in gcc12.

…stem

spoonincode · 2023-04-17T17:11:40Z

unittests/deep_mind_tests.cpp

   if(save_log)
   {
-      // Cannot use fc::copy as it does not copy to an existing destination file 
-      fc::rename(log_output.path().preferred_string(), DEEP_MIND_LOGFILE);
+      std::filesystem::copy(log_output_path, DEEP_MIND_LOGFILE, std::filesystem::copy_options::update_existing);


This fc::rename() became fs::copy(.. update_existing). The semantics are clearly different between the two. Any reason for the change?

log_output_path is derived from tmp of type temp_cfile object which would remove the file when tmp is out of scope. Simply rename the file would cause the desctructor tryiing to remove an non-existent file.

spoonincode · 2023-04-17T17:23:23Z

plugins/trace_api_plugin/configuration_utils.cpp

      if (abi_path.is_relative()) {
         abi_path = data_dir / abi_path;
      }

-      EOS_ASSERT(fc::exists(abi_path) && !fc::is_directory(abi_path), chain::plugin_config_exception, "${path} does not exist or is not a file", ("path", abi_path.generic_string()));
+      EOS_ASSERT(std::filesystem::exists(abi_path) && !std::filesystem::is_directory(abi_path), chain::plugin_config_exception, "${path} does not exist or is not a file", ("path", abi_path.generic_string()));


Thoughts on adding a to_variant(fs::path) so we don't need to put generic_string() in all these log/asserts?

+1. Although a new logging framework is likely going to make us add fmt::formatter<> for all our types.

Looks like to_variant is already provided.

spoonincode · 2023-04-17T17:49:10Z

libraries/libfc/include/fc/io/cfile.hpp

+   temp_cfile(const char* mode = "wb"){
+      std::filesystem::path template_path{ std::filesystem::temp_directory_path() / "fc-XXXXXX" };
+      char                  tmp_buf[4096];
+      strncpy(tmp_buf, template_path.c_str(), 4096);


This can result in tmp_buf lacking a null terminator.

spoonincode · 2023-04-17T18:44:42Z

programs/cleos/program_location.hpp

+#include <boost/predef/os.h>
+#include <filesystem>
+
+// adapted from boost source boost/dll/detail/posix/program_location_impl.hpp


Can we continue to use boost DLL with BOOST_DLL_USE_STD_FS, so we don't need this local reimplementation?

spoonincode · 2023-04-17T23:37:07Z

Includes
AntelopeIO/appbase#15
AntelopeIO/appbase#16
AntelopeIO/appbase#17
AntelopeIO/appbase#18
AntelopeIO/appbase#19
AntelopeIO/appbase#20
AntelopeIO/appbase#21
AntelopeIO/chainbase#5
AntelopeIO/chainbase#6
AntelopeIO/chainbase#7
AntelopeIO/chainbase#8
AntelopeIO/chainbase#9

libraries/libfc/include/fc/io/cfile.hpp

heifner · 2023-04-18T14:59:05Z

libraries/libfc/include/fc/io/json_relaxed.hpp

@@ -692,7 +690,7 @@ namespace fc { namespace json_relaxed

       FC_THROW_EXCEPTION( parse_error_exception, "expected: null|true|false" );
   }
-


Is there any way you can turn this auto removal of whitespace off in your editor?

plugins/chain_plugin/test/test_trx_retry_db.cpp

huangminghuang added 7 commits April 11, 2023 14:51

replace boost::filesystem

d15d16b

remove test_filesystem

29598be

fix compressed_file

6df0840

update submodule

67b0fca

some format fix

12d7500

update submodules

c7873a8

update appbase

69c4e00

heifner added the OCI Work exclusive to OCI team label Apr 11, 2023

stephenpdeos modified the milestone: Leap v5.0.0-rc1 Apr 12, 2023

huangminghuang added 2 commits April 12, 2023 08:32

update cmake

8bd0582

update build.yaml

6726ca6

heifner mentioned this pull request Apr 13, 2023

Mitigate unix socket path limit problem #1035

Closed

remove unused include

bdc73d4

heifner requested review from spoonincode and heifner April 14, 2023 16:09

huangminghuang added 3 commits April 14, 2023 17:52

address PR comments

c406945

remove more unnecessary generic_string()

f20f938

Merge remote-tracking branch 'origin/main' into huangminghuang/filesy…

429bb3f

…stem

spoonincode reviewed Apr 17, 2023

View reviewed changes

spoonincode approved these changes Apr 18, 2023

View reviewed changes

heifner requested changes Apr 18, 2023

View reviewed changes

address PR comments

88f477a

huangminghuang requested a review from heifner April 18, 2023 15:48

heifner approved these changes Apr 18, 2023

View reviewed changes

huangminghuang merged commit f3b9214 into main Apr 18, 2023

huangminghuang deleted the huangminghuang/filesystem branch April 18, 2023 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace boost/fc filesystem #1011

Replace boost/fc filesystem #1011

huangminghuang commented Apr 11, 2023 •

edited

Loading

spoonincode commented Apr 11, 2023

spoonincode commented Apr 11, 2023

spoonincode commented Apr 12, 2023

spoonincode Apr 17, 2023

huangminghuang Apr 17, 2023

spoonincode Apr 17, 2023

heifner Apr 17, 2023

heifner Apr 17, 2023

spoonincode Apr 17, 2023

spoonincode Apr 17, 2023

spoonincode commented Apr 17, 2023

heifner Apr 18, 2023

		@@ -692,7 +690,7 @@ namespace fc { namespace json_relaxed

		FC_THROW_EXCEPTION( parse_error_exception, "expected: null\|true\|false" );
		}

Replace boost/fc filesystem #1011

Replace boost/fc filesystem #1011

Conversation

huangminghuang commented Apr 11, 2023 • edited Loading

spoonincode commented Apr 11, 2023

spoonincode commented Apr 11, 2023

spoonincode commented Apr 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spoonincode commented Apr 17, 2023

Choose a reason for hiding this comment

huangminghuang commented Apr 11, 2023 •

edited

Loading