[pmg-devel] [PATCH proxmox-perl-rs] move apt repositories module to common

[pmg-devel] [PATCH proxmox-perl-rs] move apt repositories module to common

[Fabian Ebner]

while introducing a 'product' parameter to the relevant functions.

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---

I did the changes in Proxmox/Lib/Common.pm (and Makefile) manually.
Is that the intended way or could I have generated those somehow?

Hope I got these right:

pve-manager (respectively pmg-api) depends and build-depends on
libproxmox-rs-perl and libpve-rs-perl (respectively libpmg-rs-perl).
Both are needed, because just upgrading libproxmox-rs-perl doesn't
make the new functionality available in the product-specific shared
lib.

New libpve-rs-perl breaks old pve-manager and new libpmg-rs-perl
breaks old pmg-api.

I don't think there's a depends/breaks needed between
libproxmox-perl-rs and lib{pmg, pve}-rs-perl, but we could also go
ahead an do that, and have pve-manager and pmg-api only depend on one
of the above rather than both.

 Makefile                                   |   6 +-
 Proxmox/Lib/Common.pm                      |   8 +
 {pmg-rs => common}/src/apt/mod.rs          |   0
 {pmg-rs => common}/src/apt/repositories.rs |  18 ++-
 common/src/mod.rs                          |   1 +
 pmg-rs/src/lib.rs                          |   1 -
 pve-rs/src/apt/mod.rs                      |   1 -
 pve-rs/src/apt/repositories.rs             | 162 ---------------------
 pve-rs/src/lib.rs                          |   1 -
 9 files changed, 22 insertions(+), 176 deletions(-)
 rename {pmg-rs => common}/src/apt/mod.rs (100%)
 rename {pmg-rs => common}/src/apt/repositories.rs (86%)
 delete mode 100644 pve-rs/src/apt/mod.rs
 delete mode 100644 pve-rs/src/apt/repositories.rs

diff --git a/Makefile b/Makefile
index 6f9b597..4b59a57 100644
--- a/Makefile
+++ b/Makefile
@@ -41,13 +41,13 @@ pve pmg:
 gen:
 	$(call package_template,PMG,pmg_rs)
 	$(call package_template,PVE,pve_rs)
-	perl ./scripts/genpackage.pl Common Proxmox::RS::CalendarEvent
+	perl ./scripts/genpackage.pl Common \
+	  Proxmox::RS::APT::Repositories \
+	  Proxmox::RS::CalendarEvent
 	perl ./scripts/genpackage.pl PVE \
-	  PVE::RS::APT::Repositories \
 	  PVE::RS::OpenId \
 	  PVE::RS::TFA
 	perl ./scripts/genpackage.pl PMG \
-	  PMG::RS::APT::Repositories \
 	  PMG::RS::Acme \
 	  PMG::RS::CSR \
 	  PMG::RS::OpenId \
diff --git a/Proxmox/Lib/Common.pm b/Proxmox/Lib/Common.pm
index d8a0d57..668986d 100644
--- a/Proxmox/Lib/Common.pm
+++ b/Proxmox/Lib/Common.pm
@@ -6,6 +6,14 @@ Proxmox::Lib::Common - base module for rust bindings common between PVE and PMG
 
 =head1 SYNOPSIS
 
+    package Proxmox::RS::APT::Repositories;
+
+    use base 'Proxmox::Lib::Common';
+
+    BEGIN { __PACKAGE__->bootstrap(); }
+
+    1;
+
     package Proxmox::RS::CalendarEvent;
 
     use base 'Proxmox::Lib::Common';
diff --git a/pmg-rs/src/apt/mod.rs b/common/src/apt/mod.rs
similarity index 100%
rename from pmg-rs/src/apt/mod.rs
rename to common/src/apt/mod.rs
diff --git a/pmg-rs/src/apt/repositories.rs b/common/src/apt/repositories.rs
similarity index 86%
rename from pmg-rs/src/apt/repositories.rs
rename to common/src/apt/repositories.rs
index 596ea4d..8979b58 100644
--- a/pmg-rs/src/apt/repositories.rs
+++ b/common/src/apt/repositories.rs
@@ -1,4 +1,4 @@
-#[perlmod::package(name = "PMG::RS::APT::Repositories")]
+#[perlmod::package(name = "Proxmox::RS::APT::Repositories")]
 mod export {
     use std::convert::TryInto;
 
@@ -38,16 +38,17 @@ mod export {
         pub enabled: Option<bool>,
     }
 
-    /// Get information about configured and standard repositories.
+    /// Get information about configured repositories and standard repositories for `product`.
     #[export]
-    pub fn repositories() -> Result<RepositoriesResult, Error> {
+    pub fn repositories(product: &str) -> Result<RepositoriesResult, Error> {
         let (files, errors, digest) = proxmox_apt::repositories::repositories()?;
         let digest = hex::encode(&digest);
 
         let suite = proxmox_apt::repositories::get_current_release_codename()?;
 
         let infos = proxmox_apt::repositories::check_repositories(&files, suite);
-        let standard_repos = proxmox_apt::repositories::standard_repositories(&files, "pmg", suite);
+        let standard_repos =
+            proxmox_apt::repositories::standard_repositories(&files, product, suite);
 
         Ok(RepositoriesResult {
             files,
@@ -58,12 +59,12 @@ mod export {
         })
     }
 
-    /// Add the repository identified by the `handle`.
+    /// Add the repository identified by the `handle` and `product`.
     /// If the repository is already configured, it will be set to enabled.
     ///
     /// The `digest` parameter asserts that the configuration has not been modified.
     #[export]
-    pub fn add_repository(handle: &str, digest: Option<&str>) -> Result<(), Error> {
+    pub fn add_repository(handle: &str, product: &str, digest: Option<&str>) -> Result<(), Error> {
         let (mut files, errors, current_digest) = proxmox_apt::repositories::repositories()?;
 
         let handle: APTRepositoryHandle = handle.try_into()?;
@@ -79,7 +80,7 @@ mod export {
         // check if it's already configured first
         for file in files.iter_mut() {
             for repo in file.repositories.iter_mut() {
-                if repo.is_referenced_repository(handle, "pmg", &suite.to_string()) {
+                if repo.is_referenced_repository(handle, product, &suite.to_string()) {
                     if repo.enabled {
                         return Ok(());
                     }
@@ -92,7 +93,8 @@ mod export {
             }
         }
 
-        let (repo, path) = proxmox_apt::repositories::get_standard_repository(handle, "pmg", suite);
+        let (repo, path) =
+            proxmox_apt::repositories::get_standard_repository(handle, product, suite);
 
         if let Some(error) = errors.iter().find(|error| error.path == path) {
             bail!(
diff --git a/common/src/mod.rs b/common/src/mod.rs
index fe377d7..1beb96d 100644
--- a/common/src/mod.rs
+++ b/common/src/mod.rs
@@ -1 +1,2 @@
+mod apt;
 mod calendar_event;
diff --git a/pmg-rs/src/lib.rs b/pmg-rs/src/lib.rs
index af89416..327809e 100644
--- a/pmg-rs/src/lib.rs
+++ b/pmg-rs/src/lib.rs
@@ -2,6 +2,5 @@
 pub mod common;
 
 pub mod acme;
-pub mod apt;
 pub mod csr;
 pub mod tfa;
diff --git a/pve-rs/src/apt/mod.rs b/pve-rs/src/apt/mod.rs
deleted file mode 100644
index 574c1a7..0000000
--- a/pve-rs/src/apt/mod.rs
+++ /dev/null
@@ -1 +0,0 @@
-mod repositories;
diff --git a/pve-rs/src/apt/repositories.rs b/pve-rs/src/apt/repositories.rs
deleted file mode 100644
index 2d5e1da..0000000
--- a/pve-rs/src/apt/repositories.rs
+++ /dev/null
@@ -1,162 +0,0 @@
-#[perlmod::package(name = "PVE::RS::APT::Repositories", lib = "pve_rs")]
-mod export {
-    use std::convert::TryInto;
-
-    use anyhow::{bail, Error};
-    use serde::{Deserialize, Serialize};
-
-    use proxmox_apt::repositories::{
-        APTRepositoryFile, APTRepositoryFileError, APTRepositoryHandle, APTRepositoryInfo,
-        APTStandardRepository,
-    };
-
-    #[derive(Deserialize, Serialize)]
-    #[serde(rename_all = "kebab-case")]
-    /// Result for the repositories() function
-    pub struct RepositoriesResult {
-        /// Successfully parsed files.
-        pub files: Vec<APTRepositoryFile>,
-
-        /// Errors for files that could not be parsed or read.
-        pub errors: Vec<APTRepositoryFileError>,
-
-        /// Common digest for successfully parsed files.
-        pub digest: String,
-
-        /// Additional information/warnings about repositories.
-        pub infos: Vec<APTRepositoryInfo>,
-
-        /// Standard repositories and their configuration status.
-        pub standard_repos: Vec<APTStandardRepository>,
-    }
-
-    #[derive(Deserialize, Serialize)]
-    #[serde(rename_all = "kebab-case")]
-    /// For changing an existing repository.
-    pub struct ChangeProperties {
-        /// Whether the repository should be enabled or not.
-        pub enabled: Option<bool>,
-    }
-
-    /// Get information about configured and standard repositories.
-    #[export]
-    pub fn repositories() -> Result<RepositoriesResult, Error> {
-        let (files, errors, digest) = proxmox_apt::repositories::repositories()?;
-        let digest = hex::encode(&digest);
-
-        let suite = proxmox_apt::repositories::get_current_release_codename()?;
-
-        let infos = proxmox_apt::repositories::check_repositories(&files, suite);
-        let standard_repos = proxmox_apt::repositories::standard_repositories(&files, "pve", suite);
-
-        Ok(RepositoriesResult {
-            files,
-            errors,
-            digest,
-            infos,
-            standard_repos,
-        })
-    }
-
-    /// Add the repository identified by the `handle`.
-    /// If the repository is already configured, it will be set to enabled.
-    ///
-    /// The `digest` parameter asserts that the configuration has not been modified.
-    #[export]
-    pub fn add_repository(handle: &str, digest: Option<&str>) -> Result<(), Error> {
-        let (mut files, errors, current_digest) = proxmox_apt::repositories::repositories()?;
-
-        let handle: APTRepositoryHandle = handle.try_into()?;
-        let suite = proxmox_apt::repositories::get_current_release_codename()?;
-
-        if let Some(digest) = digest {
-            let expected_digest = hex::decode(digest)?;
-            if expected_digest != current_digest {
-                bail!("detected modified configuration - file changed by other user? Try again.");
-            }
-        }
-
-        // check if it's already configured first
-        for file in files.iter_mut() {
-            for repo in file.repositories.iter_mut() {
-                if repo.is_referenced_repository(handle, "pve", &suite.to_string()) {
-                    if repo.enabled {
-                        return Ok(());
-                    }
-
-                    repo.set_enabled(true);
-                    file.write()?;
-
-                    return Ok(());
-                }
-            }
-        }
-
-        let (repo, path) = proxmox_apt::repositories::get_standard_repository(handle, "pve", suite);
-
-        if let Some(error) = errors.iter().find(|error| error.path == path) {
-            bail!(
-                "unable to parse existing file {} - {}",
-                error.path,
-                error.error,
-            );
-        }
-
-        if let Some(file) = files.iter_mut().find(|file| file.path == path) {
-            file.repositories.push(repo);
-
-            file.write()?;
-        } else {
-            let mut file = match APTRepositoryFile::new(&path)? {
-                Some(file) => file,
-                None => bail!("invalid path - {}", path),
-            };
-
-            file.repositories.push(repo);
-
-            file.write()?;
-        }
-
-        Ok(())
-    }
-
-    /// Change the properties of the specified repository.
-    ///
-    /// The `digest` parameter asserts that the configuration has not been modified.
-    #[export]
-    pub fn change_repository(
-        path: &str,
-        index: usize,
-        options: ChangeProperties,
-        digest: Option<&str>,
-    ) -> Result<(), Error> {
-        let (mut files, errors, current_digest) = proxmox_apt::repositories::repositories()?;
-
-        if let Some(digest) = digest {
-            let expected_digest = hex::decode(digest)?;
-            if expected_digest != current_digest {
-                bail!("detected modified configuration - file changed by other user? Try again.");
-            }
-        }
-
-        if let Some(error) = errors.iter().find(|error| error.path == path) {
-            bail!("unable to parse file {} - {}", error.path, error.error);
-        }
-
-        if let Some(file) = files.iter_mut().find(|file| file.path == path) {
-            if let Some(repo) = file.repositories.get_mut(index) {
-                if let Some(enabled) = options.enabled {
-                    repo.set_enabled(enabled);
-                }
-
-                file.write()?;
-            } else {
-                bail!("invalid index - {}", index);
-            }
-        } else {
-            bail!("invalid path - {}", path);
-        }
-
-        Ok(())
-    }
-}
diff --git a/pve-rs/src/lib.rs b/pve-rs/src/lib.rs
index 26b998b..93771c2 100644
--- a/pve-rs/src/lib.rs
+++ b/pve-rs/src/lib.rs
@@ -3,6 +3,5 @@
 #[path = "../../common/src/mod.rs"]
 pub mod common;
 
-pub mod apt;
 pub mod openid;
 pub mod tfa;
-- 
2.30.2

[pmg-devel] [PATCH manager] api: apt: switch to common Proxmox::RS::APT::Repositories package

[Fabian Ebner]

which is shared between PVE and PMG.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/API2/APT.pm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/PVE/API2/APT.pm b/PVE/API2/APT.pm
index 9cf02e45..fbcd171b 100644
--- a/PVE/API2/APT.pm
+++ b/PVE/API2/APT.pm
@@ -10,6 +10,8 @@ use File::Basename;
 
 use LWP::UserAgent;
 
+use Proxmox::RS::APT::Repositories;
+
 use PVE::pvecfg;
 use PVE::Tools qw(extract_param);
 use PVE::Cluster;
@@ -19,7 +21,6 @@ use PVE::INotify;
 use PVE::Exception;
 use PVE::RESTHandler;
 use PVE::RPCEnvironment;
-use PVE::RS::APT::Repositories;
 use PVE::API2Tools;
 
 use JSON;
@@ -678,7 +679,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	return PVE::RS::APT::Repositories::repositories();
+	return Proxmox::RS::APT::Repositories::repositories("pve");
     }});
 
 __PACKAGE__->register_method({
@@ -713,7 +714,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	PVE::RS::APT::Repositories::add_repository($param->{handle}, $param->{digest});
+	Proxmox::RS::APT::Repositories::add_repository($param->{handle}, "pve", $param->{digest});
     }});
 
 __PACKAGE__->register_method({
@@ -762,7 +763,7 @@ __PACKAGE__->register_method({
 	my $enabled = $param->{enabled};
 	$options->{enabled} = int($enabled) if defined($enabled);
 
-	PVE::RS::APT::Repositories::change_repository(
+	Proxmox::RS::APT::Repositories::change_repository(
 	    $param->{path},
 	    int($param->{index}),
 	    $options,
-- 
2.30.2

[pmg-devel] [PATCH pmg-api] api: apt: switch to common Proxmox::RS::APT::Repositories package

[Fabian Ebner]

which is shared between PVE and PMG.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 src/PMG/API2/APT.pm | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/src/PMG/API2/APT.pm b/src/PMG/API2/APT.pm
index 18bb992..d6f050f 100644
--- a/src/PMG/API2/APT.pm
+++ b/src/PMG/API2/APT.pm
@@ -20,7 +20,8 @@ use PVE::JSONSchema qw(get_standard_option);
 use PMG::RESTEnvironment;
 use PMG::pmgcfg;
 use PMG::Config;
-use PMG::RS::APT::Repositories;
+
+use Proxmox::RS::APT::Repositories;
 
 use AptPkg::Cache;
 use AptPkg::Version;
@@ -668,7 +669,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	return PMG::RS::APT::Repositories::repositories();
+	return Proxmox::RS::APT::Repositories::repositories("pmg");
     }});
 
 __PACKAGE__->register_method({
@@ -701,7 +702,7 @@ __PACKAGE__->register_method({
     code => sub {
 	my ($param) = @_;
 
-	PMG::RS::APT::Repositories::add_repository($param->{handle}, $param->{digest});
+	Proxmox::RS::APT::Repositories::add_repository($param->{handle}, "pmg", $param->{digest});
     }});
 
 __PACKAGE__->register_method({
@@ -748,7 +749,7 @@ __PACKAGE__->register_method({
 	my $enabled = $param->{enabled};
 	$options->{enabled} = int($enabled) if defined($enabled);
 
-	PMG::RS::APT::Repositories::change_repository(
+	Proxmox::RS::APT::Repositories::change_repository(
 	    $param->{path},
 	    int($param->{index}),
 	    $options,
-- 
2.30.2

Re: [pmg-devel] [pve-devel] [PATCH proxmox-perl-rs] move apt repositories module to common

[Wolfgang Bumiller]

On Wed, Jul 06, 2022 at 10:54:17AM +0200, Fabian Ebner wrote:
> while introducing a 'product' parameter to the relevant functions.
> 
> Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> I did the changes in Proxmox/Lib/Common.pm (and Makefile) manually.
> Is that the intended way or could I have generated those somehow?
> 
> Hope I got these right:
> 
> pve-manager (respectively pmg-api) depends and build-depends on
> libproxmox-rs-perl and libpve-rs-perl (respectively libpmg-rs-perl).
> Both are needed, because just upgrading libproxmox-rs-perl doesn't
> make the new functionality available in the product-specific shared
> lib.
> 
> New libpve-rs-perl breaks old pve-manager and new libpmg-rs-perl
> breaks old pmg-api.

I mean in theory we could keep the old functions and just have them
forward to the common module with the corresponding product parameter
added, for a little while anyway?

> 
> I don't think there's a depends/breaks needed between
> libproxmox-perl-rs and lib{pmg, pve}-rs-perl, but we could also go
> ahead an do that, and have pve-manager and pmg-api only depend on one
> of the above rather than both.
> 
>  Makefile                                   |   6 +-
>  Proxmox/Lib/Common.pm                      |   8 +
>  {pmg-rs => common}/src/apt/mod.rs          |   0
>  {pmg-rs => common}/src/apt/repositories.rs |  18 ++-
>  common/src/mod.rs                          |   1 +
>  pmg-rs/src/lib.rs                          |   1 -
>  pve-rs/src/apt/mod.rs                      |   1 -
>  pve-rs/src/apt/repositories.rs             | 162 ---------------------
>  pve-rs/src/lib.rs                          |   1 -
>  9 files changed, 22 insertions(+), 176 deletions(-)
>  rename {pmg-rs => common}/src/apt/mod.rs (100%)
>  rename {pmg-rs => common}/src/apt/repositories.rs (86%)
>  delete mode 100644 pve-rs/src/apt/mod.rs
>  delete mode 100644 pve-rs/src/apt/repositories.rs
> 
> diff --git a/Makefile b/Makefile
> index 6f9b597..4b59a57 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -41,13 +41,13 @@ pve pmg:
>  gen:
>  	$(call package_template,PMG,pmg_rs)
>  	$(call package_template,PVE,pve_rs)
> -	perl ./scripts/genpackage.pl Common Proxmox::RS::CalendarEvent
> +	perl ./scripts/genpackage.pl Common \
> +	  Proxmox::RS::APT::Repositories \
> +	  Proxmox::RS::CalendarEvent
>  	perl ./scripts/genpackage.pl PVE \
> -	  PVE::RS::APT::Repositories \
>  	  PVE::RS::OpenId \
>  	  PVE::RS::TFA
>  	perl ./scripts/genpackage.pl PMG \
> -	  PMG::RS::APT::Repositories \
>  	  PMG::RS::Acme \
>  	  PMG::RS::CSR \
>  	  PMG::RS::OpenId \
> diff --git a/Proxmox/Lib/Common.pm b/Proxmox/Lib/Common.pm
> index d8a0d57..668986d 100644
> --- a/Proxmox/Lib/Common.pm
> +++ b/Proxmox/Lib/Common.pm
> @@ -6,6 +6,14 @@ Proxmox::Lib::Common - base module for rust bindings common between PVE and PMG
>  
>  =head1 SYNOPSIS
>  
> +    package Proxmox::RS::APT::Repositories;
> +
> +    use base 'Proxmox::Lib::Common';
> +
> +    BEGIN { __PACKAGE__->bootstrap(); }
> +
> +    1;
> +

This is just example documentation, no need to do anything to this file
at all.

>      package Proxmox::RS::CalendarEvent;
>  
>      use base 'Proxmox::Lib::Common';

Re: [pmg-devel] [pve-devel] [PATCH proxmox-perl-rs] move apt repositories module to common

[Fabian Ebner]

Am 06.07.22 um 13:13 schrieb Wolfgang Bumiller:
> On Wed, Jul 06, 2022 at 10:54:17AM +0200, Fabian Ebner wrote:
>> while introducing a 'product' parameter to the relevant functions.
>>
>> Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
>> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
>> ---
>>
>> I did the changes in Proxmox/Lib/Common.pm (and Makefile) manually.
>> Is that the intended way or could I have generated those somehow?
>>
>> Hope I got these right:
>>
>> pve-manager (respectively pmg-api) depends and build-depends on
>> libproxmox-rs-perl and libpve-rs-perl (respectively libpmg-rs-perl).
>> Both are needed, because just upgrading libproxmox-rs-perl doesn't
>> make the new functionality available in the product-specific shared
>> lib.
>>
>> New libpve-rs-perl breaks old pve-manager and new libpmg-rs-perl
>> breaks old pmg-api.
> 
> I mean in theory we could keep the old functions and just have them
> forward to the common module with the corresponding product parameter
> added, for a little while anyway?
> 

Sure, I can send a v2 doing that to postpone the "Breaks".

>> diff --git a/Proxmox/Lib/Common.pm b/Proxmox/Lib/Common.pm
>> index d8a0d57..668986d 100644
>> --- a/Proxmox/Lib/Common.pm
>> +++ b/Proxmox/Lib/Common.pm
>> @@ -6,6 +6,14 @@ Proxmox::Lib::Common - base module for rust bindings common between PVE and PMG
>>  
>>  =head1 SYNOPSIS
>>  
>> +    package Proxmox::RS::APT::Repositories;
>> +
>> +    use base 'Proxmox::Lib::Common';
>> +
>> +    BEGIN { __PACKAGE__->bootstrap(); }
>> +
>> +    1;
>> +
> 
> This is just example documentation, no need to do anything to this file
> at all.
> 

Right :)