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 <[email protected]>

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;
+			BString				fIdentifier;
-				// this URL is actually an identifier for the repository
+				// an identifier for the repository that is consistent across
-				// that is consistent across mirrors.
+				// mirrors. Usually a tag: or uuid: URI.
 			uint8				fPriority;
 			bool				fIsUserSpecific;
 

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;
+			BString				fIdentifier;
-				// This is an identifier for the repository that applies
+				// This is an identifier in the form of an URI for the
-				// across mirrors.
+				// repository, that applies across mirrors. Good choices of
+				// URI schemes are tag: and uuid:, for example.
 			BString				fVendor;
 			BString				fSummary;
 			uint8				fPriority;

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;
  				}
  			}

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",

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",
+				printf("\t\tidentifier:  %s\n",
-					repoCache.Info().URL().String());
+					repoCache.Info().Identifier().String());
 				printf("\t\torig-prio: %u\n", repoCache.Info().Priority());
 			} else
 				printf("\t\t<no repository cache found>\n");

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()) {

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;
 }
 
 

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)
+	if ((result = data->FindString(kIdentifierField, &fIdentifier)) != B_OK) {
-		return result;
+		// 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);

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()

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())
+		if (repoInfo.BaseURL().IsEmpty()) {
-			baseUrl = repoInfo.URL();
+			// legacy (pre-beta1) repositories may have a single "URL"
-		else
+			// 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);

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());
 				}