public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Christoph Heiss <c.heiss@proxmox.com>
To: Aaron Lauterer <a.lauterer@proxmox.com>
Cc: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH 00/12] installer: add crate for common code
Date: Fri, 27 Oct 2023 13:41:20 +0200	[thread overview]
Message-ID: <cc7gutf7ev742qbay3e4ses7xfnxxz3e46catyyg4n7vixzxmp@uttlzfq33evf> (raw)
In-Reply-To: <20231025160011.3617524-1-a.lauterer@proxmox.com>


Had a lot of in-person discussions with Aaron over the last few weeks
over this.
There are no real functional changes here that would impact users,
merely a code move.

Each inidivdual patch (except #1, see my answer on that) builds cleanly
on top of current master. The one dependency (see below) is also already
applied. I also did some quick smoke testing to verify everything really
still works.

I will go over the the rest of the patches afterwards, but from a glance
there is nothing that I would consider a blocker.
Things like e.g. the `InstallConfig` stuff can be done separately as
followups.

Also considering this is a great code churn, I would rather have this
get applied sooner than later, such that all future work is done on top
of this.

The only "complaint" here is that the .deb package does not build with
this series (see my reply on #1). I will shortly send a quick follow-up
patch for that, so you don't have to necessarily re-spin the whole
series just for a single, trivial line.

LGTM; thus please consider the whole series:

Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
Tested-by: Christoph Heiss <c.heiss@proxmox.com>

On Wed, Oct 25, 2023 at 05:59:59PM +0200, Aaron Lauterer wrote:
>
> since work on the auto installer is happenning in parallel, now would be
> a good point to move commonly used code into its own crate. Otherwise
> the auto-installer will always have to play catch up with the ongoing
> development of the tui installer.
>
> I tried to split up the commits as much as possible, but there are two
> larger ones, copying most the code over to the new repo and making the
> switch. The former because it is difficult to pull apart the parts that
> belong together. The latter needed to be this big as most local
> occurences needed to be removed at the same time to avoid dependency
> conflicts.
>
> One last things that is missing, is the "InstallConfig" struct.
> This should also be part of the common crate, but I need to look further
> into how to make it possible that it can be created from structs of each
> crate (tui, auto) as implementing a ::From can only be done within the
> crate where the struct lives IIUC.
>
> This series depends on the patches by Christoph to remove the global
> unsafe setup info, version 2 [0]. Without those patches applied first,
> this series will not apply.
>
> [0] https://lists.proxmox.com/pipermail/pve-devel/2023-October/059628.html
>
> Aaron Lauterer (12):
>   add proxmox-installer-common crate
>   common: copy common code from tui-installer
>   common: utils: add dependency for doc test
>   common: make InstallZfsOption public
>   common: disk_checks: make functions public
>   tui-installer: add dependency for new common crate
>   tui: switch to common crate
>   tui: remove now unused utils.rs
>   common: add installer_setup method
>   common: document installer_setup method
>   tui: use installer_setup from common cate
>   tui: remove unused read_json function
>
>  Cargo.toml                                    |   1 +
>  proxmox-installer-common/Cargo.toml           |  12 +
>  proxmox-installer-common/src/disk_checks.rs   | 237 ++++++++++
>  proxmox-installer-common/src/lib.rs           |   4 +
>  proxmox-installer-common/src/options.rs       | 387 +++++++++++++++++
>  proxmox-installer-common/src/setup.rs         | 368 ++++++++++++++++
>  .../src/utils.rs                              |   1 +
>  proxmox-tui-installer/Cargo.toml              |   1 +
>  proxmox-tui-installer/src/main.rs             |  51 +--
>  proxmox-tui-installer/src/options.rs          | 403 +-----------------
>  proxmox-tui-installer/src/setup.rs            | 324 +-------------
>  proxmox-tui-installer/src/system.rs           |   2 +-
>  proxmox-tui-installer/src/views/bootdisk.rs   | 248 +----------
>  proxmox-tui-installer/src/views/mod.rs        |   2 +-
>  proxmox-tui-installer/src/views/timezone.rs   |   4 +-
>  15 files changed, 1054 insertions(+), 991 deletions(-)
>  create mode 100644 proxmox-installer-common/Cargo.toml
>  create mode 100644 proxmox-installer-common/src/disk_checks.rs
>  create mode 100644 proxmox-installer-common/src/lib.rs
>  create mode 100644 proxmox-installer-common/src/options.rs
>  create mode 100644 proxmox-installer-common/src/setup.rs
>  rename {proxmox-tui-installer => proxmox-installer-common}/src/utils.rs (99%)
>
> --
> 2.39.2
>
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>




  parent reply	other threads:[~2023-10-27 11:41 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-10-25 15:59 Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 01/12] add proxmox-installer-common crate Aaron Lauterer
2023-10-27 10:59   ` Christoph Heiss
2023-10-25 16:00 ` [pve-devel] [PATCH 02/12] common: copy common code from tui-installer Aaron Lauterer
2023-10-27 11:14   ` Christoph Heiss
2023-10-25 16:00 ` [pve-devel] [PATCH 03/12] common: utils: add dependency for doc test Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 04/12] common: make InstallZfsOption public Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 05/12] common: disk_checks: make functions public Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 06/12] tui-installer: add dependency for new common crate Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 07/12] tui: switch to " Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 08/12] tui: remove now unused utils.rs Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 09/12] common: add installer_setup method Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 10/12] common: document " Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 11/12] tui: use installer_setup from common cate Aaron Lauterer
2023-10-25 16:00 ` [pve-devel] [PATCH 12/12] tui: remove unused read_json function Aaron Lauterer
2023-10-27 11:06   ` Christoph Heiss
2023-10-27 11:39 ` [pve-devel] [PATCH installer] buildsys: copy over `proxmox-installer-common` crate to build directory Christoph Heiss
2023-10-27 11:41 ` Christoph Heiss [this message]
2023-10-30  9:45   ` [pve-devel] [PATCH 00/12] installer: add crate for common code Aaron Lauterer
2023-11-02 19:05 ` [pve-devel] applied-series: " Thomas Lamprecht

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=cc7gutf7ev742qbay3e4ses7xfnxxz3e46catyyg4n7vixzxmp@uttlzfq33evf \
    --to=c.heiss@proxmox.com \
    --cc=a.lauterer@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal