From c68b43a6180e18689e435125a0dee4774971f4fa Mon Sep 17 00:00:00 2001 From: Bryan Henry <bryanhenry@google.com> Date: Tue, 23 Apr 2019 19:01:39 +0000 Subject: [PATCH] [appmgr] Remove deprecated-global-persistent-storage feature SEC-216 #done Change-Id: I66ed29882fe03a3412801b33de7086e211e0f920 --- build/BUILD.gn | 22 ------- ..._deprecated_global_persistent_storage.json | 20 ------- build/package.gni | 16 +---- build/package/component.gni | 20 ------- build/test/test_package.gni | 1 - .../sandbox/features/BUILD.gn | 2 +- .../meta/has_persistent_storage.cmx | 10 ---- .../{persistent-storage => storage}/BUILD.gn | 20 +------ .../has_isolated_cache_storage.cc | 0 .../has_persistent_storage.cc | 0 .../isolated_persistent_storage.cc | 0 .../meta/has_isolated_cache_storage.cmx | 0 .../meta/has_isolated_persistent_storage.cmx | 0 .../meta/isolated_persistent_storage.cmx | 0 .../meta/no_persistent_storage.cmx | 0 .../no_persistent_storage.cc | 1 + .../sandbox/sandbox_test_package.gni | 2 - .../appmgr/integration_tests/util/BUILD.gn | 1 - .../integration_tests/util/meta/util.cmx | 3 +- garnet/bin/appmgr/namespace_builder.cc | 60 ++----------------- garnet/lib/rust/cm_json/cmx_schema.json | 1 - garnet/packages/tests/BUILD.gn | 6 +- peridot/build/executable_package.gni | 1 - 23 files changed, 11 insertions(+), 175 deletions(-) delete mode 100644 build/cmx/block_deprecated_global_persistent_storage.json delete mode 100644 garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_persistent_storage.cmx rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/BUILD.gn (77%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/has_isolated_cache_storage.cc (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/has_persistent_storage.cc (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/isolated_persistent_storage.cc (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/meta/has_isolated_cache_storage.cmx (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/meta/has_isolated_persistent_storage.cmx (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/meta/isolated_persistent_storage.cmx (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/meta/no_persistent_storage.cmx (100%) rename garnet/bin/appmgr/integration_tests/sandbox/features/{persistent-storage => storage}/no_persistent_storage.cc (81%) diff --git a/build/BUILD.gn b/build/BUILD.gn index ba9980bd3e5..4f1e3c6c093 100644 --- a/build/BUILD.gn +++ b/build/BUILD.gn @@ -3,28 +3,6 @@ # found in the LICENSE file. # -group("deprecated_global_persistent_storage_allowlist") { - # ________ _________ ________ ________ - # |\ ____\|\___ ___\\ __ \|\ __ \ - # \ \ \___|\|___ \ \_\ \ \|\ \ \ \|\ \ - # \ \_____ \ \ \ \ \ \ \\\ \ \ ____\ - # \|____|\ \ \ \ \ \ \ \\\ \ \ \___| - # ____\_\ \ \ \__\ \ \_______\ \__\ - # |\_________\ \|__| \|_______|\|__| - # \|_________| - # This is an allowlist of packages that have components manifests that include - # the "deprecated-global-persistent-storage" feature. Do not add to this - # list. Use "isolated-persistent-storage" instead. - visibility = [ - # These first two entries do not need migration and will be updated when - # deleting the 'deprecated-global-persistent-storage' feature. - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:has_persistent_storage", - "//garnet/bin/appmgr/integration_tests/util:persistent_storage_test_util", - - "//garnet/bin/auth:token_manager_factory", - ] -} - group("deprecated_misc_storage_allowlist") { # ________ _________ ________ ________ # |\ ____\|\___ ___\\ __ \|\ __ \ diff --git a/build/cmx/block_deprecated_global_persistent_storage.json b/build/cmx/block_deprecated_global_persistent_storage.json deleted file mode 100644 index 58de5e3a846..00000000000 --- a/build/cmx/block_deprecated_global_persistent_storage.json +++ /dev/null @@ -1,20 +0,0 @@ -{ - "type": "object", - "$schema": "http://json-schema.org/draft-04/schema#", - "$comment": "This schema is used to whitelist access to the 'deprecate-global-persistent-storage' sandbox feature.", - "properties": { - "sandbox": { - "type": "object", - "properties": { - "features": { - "type": "array", - "items": { - "not": { - "enum": ["deprecated-global-persistent-storage"] - } - } - } - } - } - } -} diff --git a/build/package.gni b/build/package.gni index 2deb22c28e5..05c39c952af 100644 --- a/build/package.gni +++ b/build/package.gni @@ -446,7 +446,6 @@ template("package") { "binary", "components", "data_deps", - "deprecated_global_persistent_storage", "deprecated_misc_storage", "deprecated_system_image", "deps", @@ -558,16 +557,6 @@ template("package") { deps = pkg.deps public_deps = pkg.public_deps extra_schemas = [] - # TODO(bryanhenry,CF-28): Remove this validation when the - # "deprecated-global-persistent-storage" feature is removed. - if (!defined(pkg.deprecated_global_persistent_storage)) { - extra_schemas += [ - { - schema = "//build/cmx/block_deprecated_global_persistent_storage.json" - error_msg = "The 'deprecated-global-persistent-storage' feature is deprecated and guarded by an allowlist. Use 'isolated-persistent-storage' instead." - }, - ] - } # Remove this validation when the "deprecated-misc-storage" feature is removed. if (!defined(pkg.deprecated_misc_storage)) { extra_schemas += [ @@ -578,6 +567,7 @@ template("package") { ] } } + pkg.deps += [ ":$validate" ] } } @@ -892,10 +882,6 @@ template("package") { # Add the dep on the allowlist here rather than in pkg.deps earlier so that we can allow # specific targets by the package target's name using visibility. - # TODO(bryanhenry,CF-28): Remove along with deprecated-global-persistent-storage feature - if (defined(pkg.deprecated_global_persistent_storage)) { - deps += [ "${pkg.deprecated_global_persistent_storage}:deprecated_global_persistent_storage_allowlist" ] - } if (defined(pkg.deprecated_misc_storage)) { deps += [ "${pkg.deprecated_misc_storage}:deprecated_misc_storage_allowlist" ] } diff --git a/build/package/component.gni b/build/package/component.gni index 4fbeaef2bbe..69ffd2f17a8 100644 --- a/build/package/component.gni +++ b/build/package/component.gni @@ -78,7 +78,6 @@ template("fuchsia_component") { "binary", "binary_dest", "data_deps", - "deprecated_global_persistent_storage", "deps", "public_deps", "loadable_modules", @@ -149,18 +148,6 @@ template("fuchsia_component") { # but we don't know which one, so depend on all package deps here. deps = component.deps public_deps = component.public_deps - - # TODO(bryanhenry,CF-28): Remove this validation when the - # "deprecated-global-persistent-storage" feature is removed. - if (!defined(component.deprecated_global_persistent_storage)) { - extra_schemas = [ - { - schema = - "//build/cmx/block_deprecated_global_persistent_storage.json" - error_msg = "The 'deprecated-global-persistent-storage' feature is deprecated and guarded by an allowlist. Use 'isolated-persistent-storage' instead." - }, - ] - } } component.deps += [ ":$validate" ] @@ -247,13 +234,6 @@ template("fuchsia_component") { response_file_contents = manifest_args deps = component.deps public_deps = component.public_deps - - # Add the dep on the allowlist here rather than in pkg.deps earlier so that we can allow - # specific targets by the package target's name using visibility. - # TODO(bryanhenry,CF-28): Remove along with deprecated-global-persistent-storage feature - if (defined(component.deprecated_global_persistent_storage)) { - deps += [ "${component.deprecated_global_persistent_storage}:deprecated_global_persistent_storage_allowlist" ] - } } } else { group(target_name) { diff --git a/build/test/test_package.gni b/build/test/test_package.gni index 9ea6bf647f2..9db1a15dd83 100644 --- a/build/test/test_package.gni +++ b/build/test/test_package.gni @@ -125,7 +125,6 @@ template("test_package") { "binaries", "components", "data_deps", - "deprecated_global_persistent_storage", "deps", "extra", "public_deps", diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/BUILD.gn b/garnet/bin/appmgr/integration_tests/sandbox/features/BUILD.gn index 2b8f16bb29e..9ca4fa6330c 100644 --- a/garnet/bin/appmgr/integration_tests/sandbox/features/BUILD.gn +++ b/garnet/bin/appmgr/integration_tests/sandbox/features/BUILD.gn @@ -6,9 +6,9 @@ group("features") { testonly = true deps = [ "build-info", - "persistent-storage", "shell", "shell-commands", + "storage", "system-temp", ] } diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_persistent_storage.cmx b/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_persistent_storage.cmx deleted file mode 100644 index 9858f3b99f2..00000000000 --- a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_persistent_storage.cmx +++ /dev/null @@ -1,10 +0,0 @@ -{ - "program": { - "binary": "test/has_persistent_storage" - }, - "sandbox": { - "features": [ - "deprecated-global-persistent-storage" - ] - } -} diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/BUILD.gn b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/BUILD.gn similarity index 77% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/BUILD.gn rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/BUILD.gn index 822c5509851..85e16be0cb3 100644 --- a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/BUILD.gn +++ b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/BUILD.gn @@ -6,31 +6,16 @@ import("//build/test/test_package.gni") import("//build/testing/environments.gni") import("//garnet/bin/appmgr/integration_tests/sandbox/sandbox_test_package.gni") -group("persistent-storage") { +group("storage") { testonly = true deps = [ ":has_isolated_cache_storage", ":has_isolated_persistent_storage", - ":has_persistent_storage", ":isolated_persistent_storage", - ":isolated_persistent_storage_bin", ":no_persistent_storage", ] } -sandbox_test_package("has_persistent_storage") { - deprecated_global_persistent_storage = "//build" - sources = [ - "has_persistent_storage.cc", - ] - - deps = [ - "//garnet/bin/appmgr/integration_tests/sandbox:namespace_test", - "//src/lib/fxl/test:gtest_main", - ] - environments = basic_envs -} - sandbox_test_package("no_persistent_storage") { sources = [ "no_persistent_storage.cc", @@ -44,9 +29,6 @@ sandbox_test_package("no_persistent_storage") { } sandbox_test_package("has_isolated_persistent_storage") { - # This reuses the same source file as the has_persistent_storage test since it - # is very simple (just check if /data exists), but the test runs with a - # different sandbox. sources = [ "has_persistent_storage.cc", ] diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/has_isolated_cache_storage.cc b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/has_isolated_cache_storage.cc similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/has_isolated_cache_storage.cc rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/has_isolated_cache_storage.cc diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/has_persistent_storage.cc b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/has_persistent_storage.cc similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/has_persistent_storage.cc rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/has_persistent_storage.cc diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/isolated_persistent_storage.cc b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/isolated_persistent_storage.cc similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/isolated_persistent_storage.cc rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/isolated_persistent_storage.cc diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_isolated_cache_storage.cmx b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/has_isolated_cache_storage.cmx similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_isolated_cache_storage.cmx rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/has_isolated_cache_storage.cmx diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_isolated_persistent_storage.cmx b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/has_isolated_persistent_storage.cmx similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/has_isolated_persistent_storage.cmx rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/has_isolated_persistent_storage.cmx diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/isolated_persistent_storage.cmx b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/isolated_persistent_storage.cmx similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/isolated_persistent_storage.cmx rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/isolated_persistent_storage.cmx diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/no_persistent_storage.cmx b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/no_persistent_storage.cmx similarity index 100% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/meta/no_persistent_storage.cmx rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/meta/no_persistent_storage.cmx diff --git a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/no_persistent_storage.cc b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/no_persistent_storage.cc similarity index 81% rename from garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/no_persistent_storage.cc rename to garnet/bin/appmgr/integration_tests/sandbox/features/storage/no_persistent_storage.cc index 7088025ad24..e16c46772ec 100644 --- a/garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage/no_persistent_storage.cc +++ b/garnet/bin/appmgr/integration_tests/sandbox/features/storage/no_persistent_storage.cc @@ -4,4 +4,5 @@ #include "garnet/bin/appmgr/integration_tests/sandbox/namespace_test.h" +TEST_F(NamespaceTest, NoCacheStorage) { ExpectDoesNotExist("/cache/"); } TEST_F(NamespaceTest, NoPersistentStorage) { ExpectDoesNotExist("/data/"); } diff --git a/garnet/bin/appmgr/integration_tests/sandbox/sandbox_test_package.gni b/garnet/bin/appmgr/integration_tests/sandbox/sandbox_test_package.gni index f31eb975fb7..eeb26859d45 100644 --- a/garnet/bin/appmgr/integration_tests/sandbox/sandbox_test_package.gni +++ b/garnet/bin/appmgr/integration_tests/sandbox/sandbox_test_package.gni @@ -24,7 +24,6 @@ template("sandbox_test_package") { forward_variables_from(invoker, "*", [ - "deprecated_global_persistent_storage", "environments", "output_name", "target_name", @@ -33,7 +32,6 @@ template("sandbox_test_package") { } package(target_name) { - forward_variables_from(invoker, [ "deprecated_global_persistent_storage" ]) testonly = true meta = [ { diff --git a/garnet/bin/appmgr/integration_tests/util/BUILD.gn b/garnet/bin/appmgr/integration_tests/util/BUILD.gn index e2652789b8a..d18101e35f6 100644 --- a/garnet/bin/appmgr/integration_tests/util/BUILD.gn +++ b/garnet/bin/appmgr/integration_tests/util/BUILD.gn @@ -56,7 +56,6 @@ executable("persistent_storage_test_util_bin") { } package("persistent_storage_test_util") { - deprecated_global_persistent_storage = "//build" testonly = true meta = [ diff --git a/garnet/bin/appmgr/integration_tests/util/meta/util.cmx b/garnet/bin/appmgr/integration_tests/util/meta/util.cmx index ec71041abbf..51dd9ef4232 100644 --- a/garnet/bin/appmgr/integration_tests/util/meta/util.cmx +++ b/garnet/bin/appmgr/integration_tests/util/meta/util.cmx @@ -4,8 +4,7 @@ }, "sandbox": { "features": [ - "isolated-persistent-storage", - "deprecated-global-persistent-storage" + "isolated-persistent-storage" ] } } diff --git a/garnet/bin/appmgr/namespace_builder.cc b/garnet/bin/appmgr/namespace_builder.cc index 165a68c806d..8a528f46f70 100644 --- a/garnet/bin/appmgr/namespace_builder.cc +++ b/garnet/bin/appmgr/namespace_builder.cc @@ -21,55 +21,6 @@ namespace component { -namespace { - -// This function is used to migrate the existing contents of minfs into a new -// subdirectory. The subdirectory will be added to components' namespaces as -// when they request the 'deprecated-global-persistent-data' feature, in place -// of the minfs root directly. The migration allows changing the path without -// losing data across an OTA. -// TODO(CF-28): Delete this when removing 'deprecated-global-persistent-data'. -std::string MigratedGlobalPersistentDataPath() { - static const char* kGlobalPersistentDataDir = - "deprecated-global-persistent-storage"; - static const char* kDataPathsNotToMigrate[] = { - ".", "pkgfs_index", "ssh", "infra", "misc", kGlobalPersistentDataDir}; - - // Only migrate if the new directory has not been created yet, so that we only - // do it once. - const std::string new_dir(files::JoinPath("/data", kGlobalPersistentDataDir)); - if (files::IsDirectory(new_dir)) { - return new_dir; - } - - if (!files::CreateDirectory(new_dir)) { - FXL_LOG(ERROR) << "Failed to create global data directory"; - return ""; - } - - std::vector<std::string> data_paths; - if (!files::ReadDirContents("/data", &data_paths)) { - FXL_LOG(ERROR) << "Failed to read data contents"; - return ""; - } - - for (const auto& old_path : data_paths) { - if (std::find_if(std::begin(kDataPathsNotToMigrate), - std::end(kDataPathsNotToMigrate), [&](auto p) { - return old_path == std::string(p); - }) == std::end(kDataPathsNotToMigrate)) { - if (rename(files::JoinPath("/data", old_path).c_str(), - files::JoinPath(new_dir, old_path).c_str()) < 0) { - FXL_LOG(ERROR) << "Failed to migrate '" << old_path - << "' to new global data directory"; - } - } - } - return new_dir; -} - -} // namespace - NamespaceBuilder::NamespaceBuilder() = default; NamespaceBuilder::~NamespaceBuilder() = default; @@ -139,14 +90,10 @@ void NamespaceBuilder::AddSandbox( for (const auto& path : sandbox.pkgfs()) PushDirectoryFromPath("/pkgfs/" + path); - // Prioritize isolated persistent storage feature over old persistent storage - // if both included. + // Prioritize isolated persistent storage over shell feature, if both are + // present. if (sandbox.HasFeature("isolated-persistent-storage")) { PushDirectoryFromPathAs(isolated_data_path_factory(), "/data"); - } else if (sandbox.HasFeature("deprecated-global-persistent-storage")) { - // TODO(bryanhenry,CF-28): Remove this feature once users have migrated to - // isolated storage. - PushDirectoryFromPathAs(MigratedGlobalPersistentDataPath(), "/data"); } if (sandbox.HasFeature("deprecated-misc-storage")) { @@ -178,6 +125,9 @@ void NamespaceBuilder::AddSandbox( PushDirectoryFromPathAs("/pkgfs/packages/shell-commands/0/bin", "/bin"); PushDirectoryFromPath("/blob"); PushDirectoryFromPath("/boot"); + // Note that if the 'isolated-persistent-storage' feature is present, + // this is a no-op since it has handled first above. + // PushDirectoryFromPath does not override existing directories. PushDirectoryFromPath("/data"); PushDirectoryFromPath("/dev"); PushDirectoryFromChannel("/hub", hub_directory_factory()); diff --git a/garnet/lib/rust/cm_json/cmx_schema.json b/garnet/lib/rust/cm_json/cmx_schema.json index 1ff55bb9cc2..7c6b22a6ad4 100644 --- a/garnet/lib/rust/cm_json/cmx_schema.json +++ b/garnet/lib/rust/cm_json/cmx_schema.json @@ -74,7 +74,6 @@ "enum": [ "build-info", "config-data", - "deprecated-global-persistent-storage", "deprecated-misc-storage", "isolated-cache-storage", "isolated-persistent-storage", diff --git a/garnet/packages/tests/BUILD.gn b/garnet/packages/tests/BUILD.gn index 113bc1d659e..75c2347c8fb 100644 --- a/garnet/packages/tests/BUILD.gn +++ b/garnet/packages/tests/BUILD.gn @@ -1003,13 +1003,9 @@ group("runtime") { "//garnet/bin/appmgr/integration_tests/mock_runner:appmgr_mock_runner", "//garnet/bin/appmgr/integration_tests/mock_runner:fake_component_for_runner", "//garnet/bin/appmgr/integration_tests/sandbox/features/build-info:build_info_tests", - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:has_isolated_cache_storage", - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:has_isolated_persistent_storage", - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:has_persistent_storage", - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:isolated_persistent_storage", - "//garnet/bin/appmgr/integration_tests/sandbox/features/persistent-storage:no_persistent_storage", "//garnet/bin/appmgr/integration_tests/sandbox/features/shell:has_shell", "//garnet/bin/appmgr/integration_tests/sandbox/features/shell:no_shell", + "//garnet/bin/appmgr/integration_tests/sandbox/features/storage", "//garnet/bin/appmgr/integration_tests/sandbox/features/system-temp:has_system_temp", "//garnet/bin/appmgr/integration_tests/sandbox/features/system-temp:no_system_temp", "//garnet/bin/appmgr/integration_tests/sandbox/services:multiple_components", diff --git a/peridot/build/executable_package.gni b/peridot/build/executable_package.gni index e6f72b81402..02502d42bbf 100644 --- a/peridot/build/executable_package.gni +++ b/peridot/build/executable_package.gni @@ -27,7 +27,6 @@ template("executable_package") { forward_variables_from(invoker, [ "deps", - "deprecated_global_persistent_storage", "public_deps", "testonly", "meta", -- GitLab