From 41525475c533488359cb165c62af5cf131e9669e Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Fri, 22 Sep 2023 10:35:35 -0400 Subject: [PATCH] Fix chip-tool storage ifdefs to make more sense. (#29388) A few fixes here: 1) CONFIG_USE_LOCAL_STORAGE was being defined to 0 when config_use_local_storage was false, but the tests for it use #ifdef, so setting config_use_local_storage to false did not work right. 2) Commands.cpp was using config options from CHIPDeviceConfig.h without explicitly including it. It happened to be pulled in by KeyValueStoreManager.h, but better to not depend on it. 3) The UseStorageDirectory bits dependency on CHIP_DISABLE_PLATFORM_KVS was set up such that if it was true we ended up with an unused PersistedStorage::KeyValueStoreMgrImpl() return value. This got optimized out in optimized builds, so things worked, but in debug builds we would get link errors. 4) Several Darwin CI jobs that were trying to build the Debug configuraion actually built the Release one, because they did not specify a configuration and that defaults to Release. 5) Remove the unnecessary CHIP_DISABLE_PLATFORM_KVS bits from the Xcode project. --- .github/workflows/darwin-tests.yaml | 2 +- .github/workflows/darwin.yaml | 2 +- examples/chip-tool/BUILD.gn | 6 +++++- examples/chip-tool/commands/common/Commands.cpp | 8 ++++++-- src/darwin/Framework/Matter.xcodeproj/project.pbxproj | 2 -- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.github/workflows/darwin-tests.yaml b/.github/workflows/darwin-tests.yaml index cedc306cbeaf45..14a4967a39eb3e 100644 --- a/.github/workflows/darwin-tests.yaml +++ b/.github/workflows/darwin-tests.yaml @@ -73,7 +73,7 @@ jobs: # Disable -Wunguarded-availability-new because we internally use # APIs we added after our deployment target version. Maybe we # should change the deployment target version instead? - run: xcodebuild -target "darwin-framework-tool" -sdk macosx OTHER_CFLAGS='${inherited} -Wconversion -Wno-unguarded-availability-new' + run: xcodebuild -target "darwin-framework-tool" -sdk macosx -configuration Debug OTHER_CFLAGS='${inherited} -Wconversion -Wno-unguarded-availability-new' - name: Delete Defaults run: defaults delete com.apple.dt.xctest.tool continue-on-error: true diff --git a/.github/workflows/darwin.yaml b/.github/workflows/darwin.yaml index 99306feef42225..9532a4bc5d7a07 100644 --- a/.github/workflows/darwin.yaml +++ b/.github/workflows/darwin.yaml @@ -55,7 +55,7 @@ jobs: # internally use APIs that we are annotating as only available on # new enough versions. Maybe we should change out deployment # target versions instead? - run: xcodebuild -target "Matter" -sdk iphoneos OTHER_CFLAGS='${inherited} -Wno-unguarded-availability-new' + run: xcodebuild -target "Matter" -sdk iphoneos -configuration Debug OTHER_CFLAGS='${inherited} -Wno-unguarded-availability-new' - name: Run iOS Build Release working-directory: src/darwin/Framework # For now disable unguarded-availability-new warnings because we diff --git a/examples/chip-tool/BUILD.gn b/examples/chip-tool/BUILD.gn index 829b21f3f0b5cb..dd23678cccb51d 100644 --- a/examples/chip-tool/BUILD.gn +++ b/examples/chip-tool/BUILD.gn @@ -35,11 +35,15 @@ config("config") { ] defines = [ - "CONFIG_USE_LOCAL_STORAGE=${config_use_local_storage}", "CONFIG_USE_SEPARATE_EVENTLOOP=${config_use_separate_eventloop}", "CONFIG_USE_INTERACTIVE_MODE=${config_use_interactive_mode}", ] + # Note: CONFIG_USE_LOCAL_STORAGE is tested for via #ifdef, not #if. + if (config_use_local_storage) { + defines += [ "CONFIG_USE_LOCAL_STORAGE" ] + } + cflags = [ "-Wconversion" ] } diff --git a/examples/chip-tool/commands/common/Commands.cpp b/examples/chip-tool/commands/common/Commands.cpp index 050588eb7407ff..17871e12cff2e6 100644 --- a/examples/chip-tool/commands/common/Commands.cpp +++ b/examples/chip-tool/commands/common/Commands.cpp @@ -28,6 +28,7 @@ #include #include #include +#include #include #include "../clusters/JsonParser.h" @@ -42,6 +43,7 @@ constexpr const char * kJsonCommandKey = "command"; constexpr const char * kJsonCommandSpecifierKey = "command_specifier"; constexpr const char * kJsonArgumentsKey = "arguments"; +#if !CHIP_DISABLE_PLATFORM_KVS template struct HasInitWithString { @@ -61,15 +63,14 @@ struct HasInitWithString template ::value, int> = 0> static void UseStorageDirectory(T & storageManagerImpl, const char * storageDirectory) { -#if !CHIP_DISABLE_PLATFORM_KVS std::string platformKVS = std::string(storageDirectory) + "/chip_tool_kvs"; storageManagerImpl.Init(platformKVS.c_str()); -#endif // !CHIP_DISABLE_PLATFORM_KVS } template ::value, int> = 0> static void UseStorageDirectory(T & storageManagerImpl, const char * storageDirectory) {} +#endif // !CHIP_DISABLE_PLATFORM_KVS bool GetArgumentsFromJson(Command * command, Json::Value & value, bool optional, std::vector & outArgs) { @@ -321,7 +322,10 @@ CHIP_ERROR Commands::RunCommand(int argc, char ** argv, bool interactive, chip::Logging::SetLogFilter(mStorage.GetLoggingLevel()); +#if !CHIP_DISABLE_PLATFORM_KVS UseStorageDirectory(chip::DeviceLayer::PersistedStorage::KeyValueStoreMgrImpl(), mStorage.GetDirectory()); +#endif // !CHIP_DISABLE_PLATFORM_KVS + #endif // CONFIG_USE_LOCAL_STORAGE return command->Run(); diff --git a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj index 400976afbc5339..ff65f91933836a 100644 --- a/src/darwin/Framework/Matter.xcodeproj/project.pbxproj +++ b/src/darwin/Framework/Matter.xcodeproj/project.pbxproj @@ -1827,7 +1827,6 @@ CONFIG_BUILD_FOR_HOST_UNIT_TEST, "CHIP_CONFIG_SKIP_APP_SPECIFIC_GENERATED_HEADER_INCLUDES=1", "CONFIG_USE_INTERACTIVE_MODE=1", - "CHIP_DISABLE_PLATFORM_KVS=1", ); "HEADER_SEARCH_PATHS[arch=*]" = ( "$(CHIP_ROOT)/examples/darwin-framework-tool", @@ -1897,7 +1896,6 @@ CONFIG_BUILD_FOR_HOST_UNIT_TEST, "CHIP_CONFIG_SKIP_APP_SPECIFIC_GENERATED_HEADER_INCLUDES=1", "CONFIG_USE_INTERACTIVE_MODE=1", - "CHIP_DISABLE_PLATFORM_KVS=1", ); "HEADER_SEARCH_PATHS[arch=*]" = ( "$(CHIP_ROOT)/examples//darwin-framework-tool",