Skip to content

Commit

Permalink
Fix chip-tool storage ifdefs to make more sense. (#29388)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Oct 11, 2023
1 parent 6c7b7c0 commit 4152547
Show file tree
Hide file tree
Showing 5 changed files with 13 additions and 7 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/darwin-tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/darwin.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 5 additions & 1 deletion examples/chip-tool/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -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" ]
}

Expand Down
8 changes: 6 additions & 2 deletions examples/chip-tool/commands/common/Commands.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
#include <lib/support/Base64.h>
#include <lib/support/CHIPMem.h>
#include <lib/support/CodeUtils.h>
#include <platform/CHIPDeviceConfig.h>
#include <platform/KeyValueStoreManager.h>

#include "../clusters/JsonParser.h"
Expand All @@ -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 <typename T>
struct HasInitWithString
{
Expand All @@ -61,15 +63,14 @@ struct HasInitWithString
template <typename T, std::enable_if_t<HasInitWithString<T>::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 <typename T, std::enable_if_t<!HasInitWithString<T>::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<std::string> & outArgs)
{
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 0 additions & 2 deletions src/darwin/Framework/Matter.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down

0 comments on commit 4152547

Please sign in to comment.