From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id CDF469DA40 for ; Fri, 27 Oct 2023 13:41:52 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id ABAEE38F50 for ; Fri, 27 Oct 2023 13:41:22 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Fri, 27 Oct 2023 13:41:21 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B93FA42549 for ; Fri, 27 Oct 2023 13:41:21 +0200 (CEST) Date: Fri, 27 Oct 2023 13:41:20 +0200 From: Christoph Heiss To: Aaron Lauterer Cc: Proxmox VE development discussion Message-ID: References: <20231025160011.3617524-1-a.lauterer@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20231025160011.3617524-1-a.lauterer@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL -0.020 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [mod.rs, system.rs, proxmox.com, lib.rs, timezone.rs, bootdisk.rs, options.rs, main.rs, utils.rs, setup.rs] Subject: Re: [pve-devel] [PATCH 00/12] installer: add crate for common code X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 27 Oct 2023 11:41:52 -0000 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 Tested-by: Christoph Heiss 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 > >