From aa272ca35cc27142fcc9450622f336755342c9b5 Mon Sep 17 00:00:00 2001 From: Adrien Destugues Date: Fri, 1 May 2020 14:38:08 +0200 Subject: [PATCH] Package Kit: reduce identifer/base-url confusion For historical reasons, the package kit has an "url" field that is not actually meant to be used as an URL. Rename it in the API and user facing output as "identifier" to make it clear what the file is used for. This change preserves the "url" key in on-disk and online storage (hpkr files, stored settings, etc) in an attempt to not break anything. Fix one remaining misuse of the "url" field as an URL in get_package_dependencies. Add an unit test showing that BUrl does parse "tab" URIs properly (there is just a protocol and a path segment). Change-Id: I339ce526e5798d42d78ae650855d7e988dbb4a1a Reviewed-on: https://review.haiku-os.org/c/haiku/+/2542 Reviewed-by: waddlesplash --- headers/os/package/RepositoryConfig.h | 10 ++-- headers/os/package/RepositoryInfo.h | 12 +++-- .../server/LocalPkgDataLoadProcess.cpp | 6 +-- src/bin/package_repo/command_list.cpp | 2 +- src/bin/pkgman/command_list_repos.cpp | 6 +-- .../package/ActivateRepositoryConfigJob.cpp | 2 +- src/kits/package/RepositoryConfig.cpp | 18 +++---- src/kits/package/RepositoryInfo.cpp | 47 ++++++++++++++----- src/tests/kits/net/service/UrlTest.cpp | 4 +- .../create_repository_config.cpp | 10 ++-- .../get_package_dependencies.cpp | 2 +- 11 files changed, 73 insertions(+), 46 deletions(-) diff --git a/headers/os/package/RepositoryConfig.h b/headers/os/package/RepositoryConfig.h index 3d798a1bc5..6a3afbb8f0 100644 --- a/headers/os/package/RepositoryConfig.h +++ b/headers/os/package/RepositoryConfig.h @@ -28,7 +28,7 @@ public: const BString& Name() const; const BString& BaseURL() const; - const BString& URL() const; + const BString& Identifier() const; uint8 Priority() const; bool IsUserSpecific() const; @@ -38,7 +38,7 @@ public: void SetName(const BString& name); void SetBaseURL(const BString& url); - void SetURL(const BString& url); + void SetIdentifier(const BString& url); void SetPriority(uint8 priority); void SetIsUserSpecific(bool isUserSpecific); @@ -52,9 +52,9 @@ private: BString fBaseURL; // this URL is the URL that can be used to access the data of // the repository - it points to a single mirror. - BString fURL; - // this URL is actually an identifier for the repository - // that is consistent across mirrors. + BString fIdentifier; + // an identifier for the repository that is consistent across + // mirrors. Usually a tag: or uuid: URI. uint8 fPriority; bool fIsUserSpecific; diff --git a/headers/os/package/RepositoryInfo.h b/headers/os/package/RepositoryInfo.h index 9b7f7396c6..c17fa04e38 100644 --- a/headers/os/package/RepositoryInfo.h +++ b/headers/os/package/RepositoryInfo.h @@ -34,7 +34,7 @@ public: const BString& Name() const; const BString& BaseURL() const; - const BString& URL() const; + const BString& Identifier() const; const BString& Vendor() const; const BString& Summary() const; uint8 Priority() const; @@ -44,7 +44,7 @@ public: void SetName(const BString& name); void SetBaseURL(const BString& url); - void SetURL(const BString& url); + void SetIdentifier(const BString& url); void SetVendor(const BString& vendor); void SetSummary(const BString& summary); void SetPriority(uint8 priority); @@ -61,6 +61,7 @@ public: static const char* const kNameField; static const char* const kURLField; + static const char* const kIdentifierField; static const char* const kBaseURLField; static const char* const kVendorField; static const char* const kSummaryField; @@ -79,9 +80,10 @@ private: BString fName; BString fBaseURL; // This is the URL to a single mirror. This field is optional. - BString fURL; - // This is an identifier for the repository that applies - // across mirrors. + BString fIdentifier; + // This is an identifier in the form of an URI for the + // repository, that applies across mirrors. Good choices of + // URI schemes are tag: and uuid:, for example. BString fVendor; BString fSummary; uint8 fPriority; diff --git a/src/apps/haikudepot/server/LocalPkgDataLoadProcess.cpp b/src/apps/haikudepot/server/LocalPkgDataLoadProcess.cpp index 47c01de7d1..55e89e72ab 100644 --- a/src/apps/haikudepot/server/LocalPkgDataLoadProcess.cpp +++ b/src/apps/haikudepot/server/LocalPkgDataLoadProcess.cpp @@ -110,12 +110,12 @@ LocalPkgDataLoadProcess::RunInternal() repoName, &repoConfig); if (getRepositoryConfigStatus == B_OK) { - depotInfo.SetURL(repoConfig.URL()); + depotInfo.SetURL(repoConfig.Identifier()); if (Logger::IsDebugEnabled()) { printf("[%s] local repository [%s] info;\n" " * url [%s]\n", Name(), repoName.String(), - repoConfig.URL().String()); + repoConfig.Identifier().String()); } } else { printf("[%s] unable to obtain the repository config for local " @@ -220,7 +220,7 @@ LocalPkgDataLoadProcess::RunInternal() for (it = depots.begin(); it != depots.end(); it++) { if (RepositoryUrlUtils::EqualsNormalized( - it->URL(), remoteRepository->Config().URL())) { + it->URL(), remoteRepository->Config().Identifier())) { break; } } diff --git a/src/bin/package_repo/command_list.cpp b/src/bin/package_repo/command_list.cpp index bcb5ede6de..c7e39facff 100644 --- a/src/bin/package_repo/command_list.cpp +++ b/src/bin/package_repo/command_list.cpp @@ -70,7 +70,7 @@ struct RepositoryContentListHandler : BRepositoryContentHandler { printf("\tname: %s\n", repositoryInfo.Name().String()); printf("\tsummary: %s\n", repositoryInfo.Summary().String()); printf("\tbase-url: %s\n", repositoryInfo.BaseURL().String()); - printf("\turl: %s\n", repositoryInfo.URL().String()); + printf("\tidentifier: %s\n", repositoryInfo.Identifier().String()); printf("\tvendor: %s\n", repositoryInfo.Vendor().String()); printf("\tpriority: %u\n", repositoryInfo.Priority()); printf("\tarchitecture: %s\n", diff --git a/src/bin/pkgman/command_list_repos.cpp b/src/bin/pkgman/command_list_repos.cpp index 56a10aeeb5..cb96c75bc4 100644 --- a/src/bin/pkgman/command_list_repos.cpp +++ b/src/bin/pkgman/command_list_repos.cpp @@ -104,7 +104,7 @@ ListReposCommand::Execute(int argc, const char* const* argv) repoConfig.IsUserSpecific() ? "[User]" : " ", repoConfig.Name().String()); printf("\t\tbase-url: %s\n", repoConfig.BaseURL().String()); - printf("\t\turl: %s\n", repoConfig.URL().String()); + printf("\t\tidentifier: %s\n", repoConfig.Identifier().String()); printf("\t\tpriority: %u\n", repoConfig.Priority()); if (verbose) { @@ -121,8 +121,8 @@ ListReposCommand::Execute(int argc, const char* const* argv) repoCache.CountPackages()); printf("\t\tbase-url: %s\n", repoCache.Info().BaseURL().String()); - printf("\t\turl: %s\n", - repoCache.Info().URL().String()); + printf("\t\tidentifier: %s\n", + repoCache.Info().Identifier().String()); printf("\t\torig-prio: %u\n", repoCache.Info().Priority()); } else printf("\t\t\n"); diff --git a/src/kits/package/ActivateRepositoryConfigJob.cpp b/src/kits/package/ActivateRepositoryConfigJob.cpp index 23ff6f2464..7adb55698b 100644 --- a/src/kits/package/ActivateRepositoryConfigJob.cpp +++ b/src/kits/package/ActivateRepositoryConfigJob.cpp @@ -67,7 +67,7 @@ ActivateRepositoryConfigJob::Execute() BRepositoryConfig repoConfig; repoConfig.SetName(repoInfo.Name()); repoConfig.SetBaseURL(fRepositoryBaseURL); - repoConfig.SetURL(repoInfo.URL()); + repoConfig.SetIdentifier(repoInfo.Identifier()); repoConfig.SetPriority(repoInfo.Priority()); if (fRepositoryBaseURL.IsEmpty()) { diff --git a/src/kits/package/RepositoryConfig.cpp b/src/kits/package/RepositoryConfig.cpp index 2f40083a68..b4d61d80a1 100644 --- a/src/kits/package/RepositoryConfig.cpp +++ b/src/kits/package/RepositoryConfig.cpp @@ -85,10 +85,10 @@ BRepositoryConfig::Store(const BEntry& entry) const configString << "# This URL is an identifier for the repository that is " "consistent across mirrors\n"; - if (fURL.IsEmpty()) + if (fIdentifier.IsEmpty()) configString << "# " << KEY_URL << "=???\n"; else - configString << KEY_URL << "=" << fURL << "\n"; + configString << KEY_URL << "=" << fIdentifier << "\n"; configString << "\n"; configString << KEY_PRIORITY << "=" << fPriority << "\n"; @@ -123,14 +123,14 @@ BRepositoryConfig::SetTo(const BEntry& entry) if (result != B_OK) return result; - const char* url = NULL; + const char* identifier = NULL; const char* version = driverSettings.GetParameterValue(KEY_CONFIG_VERSION); const char *baseUrlKey = KEY_BASE_URL; if (version == NULL || atoi(version) < 2) baseUrlKey = KEY_BASE_URL_LEGACY; else - url = driverSettings.GetParameterValue(KEY_URL); + identifier = driverSettings.GetParameterValue(KEY_URL); const char* baseUrl = driverSettings.GetParameterValue(baseUrlKey); const char* priorityString = driverSettings.GetParameterValue(KEY_PRIORITY); @@ -142,7 +142,7 @@ BRepositoryConfig::SetTo(const BEntry& entry) fBaseURL = baseUrl; fPriority = priorityString == NULL ? kUnsetPriority : atoi(priorityString); - fURL = url == NULL ? "" : url; + fIdentifier = identifier == NULL ? "" : identifier; BPath userSettingsPath; if (find_directory(B_USER_SETTINGS_DIRECTORY, &userSettingsPath) == B_OK) { @@ -172,9 +172,9 @@ BRepositoryConfig::BaseURL() const const BString& -BRepositoryConfig::URL() const +BRepositoryConfig::Identifier() const { - return fURL; + return fIdentifier; } @@ -223,9 +223,9 @@ BRepositoryConfig::SetBaseURL(const BString& baseURL) void -BRepositoryConfig::SetURL(const BString& URL) +BRepositoryConfig::SetIdentifier(const BString& identifier) { - fURL = URL; + fIdentifier = identifier; } diff --git a/src/kits/package/RepositoryInfo.cpp b/src/kits/package/RepositoryInfo.cpp index 09c722c96d..e43ace75ba 100644 --- a/src/kits/package/RepositoryInfo.cpp +++ b/src/kits/package/RepositoryInfo.cpp @@ -29,6 +29,7 @@ const uint8 BRepositoryInfo::kDefaultPriority = 50; const char* const BRepositoryInfo::kNameField = "name"; const char* const BRepositoryInfo::kURLField = "url"; +const char* const BRepositoryInfo::kIdentifierField = "identifier"; const char* const BRepositoryInfo::kBaseURLField = "baseUrl"; const char* const BRepositoryInfo::kVendorField = "vendor"; const char* const BRepositoryInfo::kSummaryField = "summary"; @@ -91,7 +92,10 @@ BRepositoryInfo::Archive(BMessage* data, bool deep) const if ((result = data->AddString(kNameField, fName)) != B_OK) return result; - if ((result = data->AddString(kURLField, fURL)) != B_OK) + // Field in the archive is named "url" for backward compatility reasons. + // We can change this when everyone has updated to a version of Haiku + // with support for reading the "identifier" field. + if ((result = data->AddString(kURLField, fIdentifier)) != B_OK) return result; if ((result = data->AddString(kVendorField, fVendor)) != B_OK) return result; @@ -153,9 +157,9 @@ BRepositoryInfo::BaseURL() const const BString& -BRepositoryInfo::URL() const +BRepositoryInfo::Identifier() const { - return fURL; + return fIdentifier; } @@ -209,9 +213,9 @@ BRepositoryInfo::SetName(const BString& name) void -BRepositoryInfo::SetURL(const BString& url) +BRepositoryInfo::SetIdentifier(const BString& identifier) { - fURL = url; + fIdentifier = identifier; } @@ -275,14 +279,16 @@ BRepositoryInfo::_SetTo(const BMessage* data) if (data == NULL) return B_BAD_VALUE; - data->FindString(kBaseURLField, &fBaseURL); - // optional value for historical reasons - status_t result; if ((result = data->FindString(kNameField, &fName)) != B_OK) return result; - if ((result = data->FindString(kURLField, &fURL)) != B_OK) - return result; + if ((result = data->FindString(kIdentifierField, &fIdentifier)) != B_OK) { + // Handle the "url" field as well (it is still the one we generate). + // Later on when everyone is using this code we can switch the writing + // side to use the "identifier" field with its correct name. + if ((result = data->FindString(kURLField, &fIdentifier)) != B_OK) + return result; + } if ((result = data->FindString(kVendorField, &fVendor)) != B_OK) return result; if ((result = data->FindString(kSummaryField, &fSummary)) != B_OK) @@ -295,6 +301,17 @@ BRepositoryInfo::_SetTo(const BMessage* data) if (fArchitecture == B_PACKAGE_ARCHITECTURE_ANY) return B_BAD_DATA; + // Old packages had no base-url field, the "url" field acted both as an + // identifier and locator for the repository. + data->FindString(kBaseURLField, &fBaseURL); + if (fBaseURL.Length() == 0) { + fBaseURL = fIdentifier; + // In that case make sure the identifier is indeed an http URL + // (in the new format, the protocol is not required to be http anymore) + if (!fBaseURL.StartsWith("http")) + return B_BAD_DATA; + } + const char* licenseName; const char* licenseText; for (int i = 0; @@ -341,7 +358,10 @@ BRepositoryInfo::_SetTo(const BEntry& entry) &unload_driver_settings); const char* name = get_driver_parameter(settingsHandle, "name", NULL, NULL); - const char* url = get_driver_parameter(settingsHandle, "url", NULL, NULL); + const char* identifier = get_driver_parameter(settingsHandle, "identifier", NULL, NULL); + // Also handle the old name if the new one isn't found + if (identifier == NULL || *identifier == '\0') + identifier = get_driver_parameter(settingsHandle, "url", NULL, NULL); const char* baseUrl = get_driver_parameter(settingsHandle, "baseurl", NULL, NULL); const char* vendor = get_driver_parameter(settingsHandle, "vendor", NULL, NULL); @@ -352,7 +372,8 @@ BRepositoryInfo::_SetTo(const BEntry& entry) const char* architectureString = get_driver_parameter(settingsHandle, "architecture", NULL, NULL); - if (name == NULL || *name == '\0' || url == NULL || *url == '\0' + if (name == NULL || *name == '\0' + || identifier == NULL || *identifier == '\0' || vendor == NULL || *vendor == '\0' || summary == NULL || *summary == '\0' || priorityString == NULL || *priorityString == '\0' @@ -368,7 +389,7 @@ BRepositoryInfo::_SetTo(const BEntry& entry) fName = name; fBaseURL = baseUrl; - fURL = url; + fIdentifier = identifier; fVendor = vendor; fSummary = summary; fPriority = atoi(priorityString); diff --git a/src/tests/kits/net/service/UrlTest.cpp b/src/tests/kits/net/service/UrlTest.cpp index 90c51cce5f..4ec025937a 100644 --- a/src/tests/kits/net/service/UrlTest.cpp +++ b/src/tests/kits/net/service/UrlTest.cpp @@ -202,7 +202,9 @@ const ExplodeTest kTestExplode[] = { "/boot/home/Desktop/index.html", { "", "", "", "", 0, "/boot/home/Desktop/index.html","",""} }, { "//remote.host/boot/home/Desktop", - { "", "", "", "remote.host", 0, "/boot/home/Desktop","",""} } + { "", "", "", "remote.host", 0, "/boot/home/Desktop","",""} }, + { "tag:haiku-os.org,2020:repositories/haiku/r1beta2/x86_gcc2", + { "tag", "", "", "", 0, "haiku-os.org,2020:repositories/haiku/r1beta2/x86_gcc2" } } }; void UrlTest::ExplodeImplodeTest() diff --git a/src/tools/create_repository_config/create_repository_config.cpp b/src/tools/create_repository_config/create_repository_config.cpp index fda2939afc..f69cd79e0d 100644 --- a/src/tools/create_repository_config/create_repository_config.cpp +++ b/src/tools/create_repository_config/create_repository_config.cpp @@ -70,9 +70,11 @@ main(int argc, const char* const* argv) "failed to read repository info file \"%s\"", infoPath); if (baseUrl == NULL) { - if (repoInfo.BaseURL().IsEmpty()) - baseUrl = repoInfo.URL(); - else + if (repoInfo.BaseURL().IsEmpty()) { + // legacy (pre-beta1) repositories may have a single "URL" + // field acting both as baseURL and identifier. + baseUrl = repoInfo.Identifier(); + } else baseUrl = repoInfo.BaseURL(); } @@ -80,7 +82,7 @@ main(int argc, const char* const* argv) BPackageKit::BRepositoryConfig repoConfig; repoConfig.SetName(repoInfo.Name()); repoConfig.SetBaseURL(baseUrl); - repoConfig.SetURL(repoInfo.URL()); + repoConfig.SetIdentifier(repoInfo.Identifier()); repoConfig.SetPriority(repoInfo.Priority()); DIE_ON_ERROR(repoConfig.Store(configPath), "failed to write repository config file \"%s\"", configPath); diff --git a/src/tools/get_package_dependencies/get_package_dependencies.cpp b/src/tools/get_package_dependencies/get_package_dependencies.cpp index e016d0a4a7..31802af200 100644 --- a/src/tools/get_package_dependencies/get_package_dependencies.cpp +++ b/src/tools/get_package_dependencies/get_package_dependencies.cpp @@ -155,7 +155,7 @@ main(int argc, const char* const* argv) if (package->Repository() != &installedRepository) { const BRepositoryInfo& info = repositoryInfos[package->Repository()]; - BString url = info.URL(); + BString url = info.BaseURL(); url << "/packages/" << package->Info().CanonicalFileName(); printf("%s\n", url.String()); }