From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mp10.migadu.com ([2001:41d0:403:478a::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by ms9.migadu.com with LMTPS id SHrXEP2PvWQRUwEASxT56A (envelope-from ) for ; Sun, 23 Jul 2023 22:39:25 +0200 Received: from aspmx1.migadu.com ([2001:41d0:403:478a::]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)) by mp10.migadu.com with LMTPS id aPz+D/2PvWS1NgEAG6o9tA (envelope-from ) for ; Sun, 23 Jul 2023 22:39:25 +0200 Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by aspmx1.migadu.com (Postfix) with ESMTPS id 5D4296367D for ; Sun, 23 Jul 2023 22:39:24 +0200 (CEST) Authentication-Results: aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=incana.org header.s=gm1 header.b=XrCbOmaf; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org"; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=yhetil.org; s=key1; t=1690144765; h=from:from:sender:sender:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:resent-cc:resent-from:resent-sender: resent-message-id:in-reply-to:in-reply-to:references:references: list-id:list-help:list-unsubscribe:list-subscribe:list-post: dkim-signature; bh=gKOTfBL2ur4O0KrIRsSv/mbTibfkRKIf/a3X2DoLRYc=; b=AUcVTeVRZoWbKM1gST+I4sZwxNcm8zZCTWuE76ExSkb3+KJsTJPwv79JedoqcoFslR8wm6 HFaghgsJ/yVEk7tZ+cdGLAX8og2qOAOcU2JCY208NiHfYqAFgNDB1Y0LLdnDKcXe9nExPr jZzJ/abEql8mRAJfRdexaM7oazxztpp+3HhgZrJNEs8BmMolFvQzqSi7/1CRQDG8U8BGUC VxV6D5LVbjXbgCpMfnW5hgWhY/3QdI58OLvkw1VSRF4U3zuaJiAK2yzHXEHw53DYpyWPFw sS9Su0HSoLod9bRN0lKFY9EyAXDKMGKe8E6tolfnRa28w7iiwPAbKuOC5ZJwug== ARC-Authentication-Results: i=1; aspmx1.migadu.com; dkim=fail ("headers rsa verify failed") header.d=incana.org header.s=gm1 header.b=XrCbOmaf; spf=pass (aspmx1.migadu.com: domain of "guix-patches-bounces+larch=yhetil.org@gnu.org" designates 209.51.188.17 as permitted sender) smtp.mailfrom="guix-patches-bounces+larch=yhetil.org@gnu.org"; dmarc=none ARC-Seal: i=1; s=key1; d=yhetil.org; t=1690144765; a=rsa-sha256; cv=none; b=hi8LKfx33vH4nj98KpyYauOn0o0VUhpfHTuY6CMFnf48twQrm+ZF/wpjfnXed53JRnzaSk X0iuUywXaDPr4HudmUFzSVrMei90tNGR33aRFl1YDtfT4ufMzpqwWz5jPe1yi8WjFmt3B9 Ol1ggHqDg0Q1oyqJ8Syj57VAs0X69Q8W3pGmpwGHg0KqhpT7OJrCnw9xMZBU2JGW1WzDJ2 kY/BkOd4HtPe8guozlnkGmo79jqP9Vzbc1ZB1H+43vgY3zwzqXN5pLeM86MA9Dia8qMkTv ViaPoVPICWYAXFnEf/Xx2g/6XoVDUEYw+su4OjV/oO1YDIv2s/1/hjFuBd274g== Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1qNfr7-0004mm-EN; Sun, 23 Jul 2023 16:39:05 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1qNfr4-0004mJ-N5 for guix-patches@gnu.org; Sun, 23 Jul 2023 16:39:02 -0400 Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1qNfr4-0004KG-Fo for guix-patches@gnu.org; Sun, 23 Jul 2023 16:39:02 -0400 Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1qNfr4-0006s7-CP for guix-patches@gnu.org; Sun, 23 Jul 2023 16:39:02 -0400 X-Loop: help-debbugs@gnu.org Subject: [bug#64804] [PATCH] gnu: rust: Update to Rust 1.71.0 References: <1bbbeed9c7c6e50464bee042e8dd06e9cd62c4c2.1690094273.git.fries1234@protonmail.com> In-Reply-To: <1bbbeed9c7c6e50464bee042e8dd06e9cd62c4c2.1690094273.git.fries1234@protonmail.com> Resent-From: Juliana Sims Original-Sender: "Debbugs-submit" Resent-CC: guix-patches@gnu.org Resent-Date: Sun, 23 Jul 2023 20:39:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 64804 X-GNU-PR-Package: guix-patches X-GNU-PR-Keywords: patch To: 64804@debbugs.gnu.org Cc: fries1234@protonmail.com Received: via spool by 64804-submit@debbugs.gnu.org id=B64804.169014471226366 (code B ref 64804); Sun, 23 Jul 2023 20:39:02 +0000 Received: (at 64804) by debbugs.gnu.org; 23 Jul 2023 20:38:32 +0000 Received: from localhost ([127.0.0.1]:41307 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qNfqZ-0006rB-MF for submit@debbugs.gnu.org; Sun, 23 Jul 2023 16:38:32 -0400 Received: from relay4-d.mail.gandi.net ([2001:4b98:dc4:8::224]:51377) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1qNfqW-0006qm-M0 for 64804@debbugs.gnu.org; Sun, 23 Jul 2023 16:38:31 -0400 Received: by mail.gandi.net (Postfix) with ESMTPSA id 3D38DE0002; Sun, 23 Jul 2023 20:38:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=incana.org; s=gm1; t=1690144702; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type; bh=gKOTfBL2ur4O0KrIRsSv/mbTibfkRKIf/a3X2DoLRYc=; b=XrCbOmaftHhyozjMrIfz4nBsKaRJiaqsdF0Qb4qhSqHbKRGWxTsyjpgFGe1wc3L0t58iLL C1edPmA/kTuvjn1otAN+26A8oXuSFQjseO8PfsL81DSsXcy/fvaSTGd/tfyQxKHVs7Qto9 NofCAsiMmDYYm7Ma0jbzj1Iow2OSoxbZ+5nJ/0z5HXc1YUN8S1g3pimVtWfXu153EGySuf FnNwY5DExPR1r5+WXljuMfk3N0OaHSCbBPOsCO8aomNGEXPmAwD6AfM2ntEtCSFVaH6zgW zEo5uhYD42agYrPRK1m0QUslz0GM0uiZVeMUy8L4PcZcU+ERBIQpiNihiZXJsg== Date: Sun, 23 Jul 2023 16:38:10 -0400 From: Juliana Sims Message-Id: X-Mailer: geary/43.0 MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed X-GND-Sasl: juli@incana.org X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: guix-patches@gnu.org List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: 1bbbeed9c7c6e50464bee042e8dd06e9cd62c4c2.1690094273.git.fries1234@protonmail.com Errors-To: guix-patches-bounces+larch=yhetil.org@gnu.org Sender: guix-patches-bounces+larch=yhetil.org@gnu.org X-Migadu-Flow: FLOW_IN X-Migadu-Country: US X-Spam-Score: 0.75 X-Migadu-Queue-Id: 5D4296367D X-Migadu-Spam-Score: 0.75 X-Migadu-Scanner: mx0.migadu.com X-TUID: YIwEQPirQGq7 Hi Fries, While I'm not a Rust packager (and therefore may miss some stuff during review), there are a few glaring issues which stand out for me. Also forgive the formatting; not having the message proper to reply to complicates things. First and foremost, Rust has 11052 reverse dependencies and this patch would rebuild 25834 packages. This should be submitted to the core-updates branch. I'm not sure whether this patch should be resubmitted for it or if others can just keep this in mind when considering merging it. The next major thing is that this should be split into multiple patches. Make each discrete new Rust in its own patch, for example. Ideally, each patch would be atomic so that only a subset could be applied and packages would still build fine. If I were doing it, I would commit each intermediate version of Rust and bump the default Rust version with that commit. You may also be able to get away with adding each intermediate Rust and only bumping the version of the default rust for the last one; others will have to weigh in. See Contributing > Submitting Patches > Sending a Patch Series > Multiple Patches in the manual for how to submit such a patch series if you need it. Make sure your commit messages match the proper style; see https://www.gnu.org/prep/standards/html_node/Change-Logs.html#Change-Logs and the commit history. There's one point where you add three new, empty lines near the beginning of the patch. Prune those. Where you write new modify-phases forms and new snippets, use g-expressions. > + [target."cfg(windows)".dependencies.windows-sys] > + version = "0.45.0" Shouldn't we be able to remove this altogether somehow since we're not the target operating system? > + ;; Rust 1.70 uses the rustix library which on Linux, it defaults to > + ;; using outline ASM which without the cc cargo feature enabled, it > + ;; will expect a precompiled binary library. This patch will enable the cargo > + ;; cc feature flag inside the fd-lock vendored Cargo.toml file, which is the > + ;; crate that uses rustix. This comment is difficult to parse. Could it be reworded more clearly? Perhaps, "Rust 1.70 adds the rustix library which uses outline ASM. The vendored fd-lock crate uses rustix. It expects a precompiled binary library without the "cc" Cargo feature enabled. This patch enables the "cc" feature flag inside the vendored fd-lock Cargo.toml file." That said, fd-lock is already packaged in Guix; the vendored version should be avoided. > + (("(checksum = )\".*\"" all name) > + (string-append name "\"" ,%cargo-reference-hash "\""))) Clever. > + ;;; Function to make creating a list to ignore tests a bit easier. This should probably be a docstring instead of a comment, and it should describe what the function does - "Accept a list of strings containing test names, and return a list of forms for skipping those tests" maybe. > + (define (make-ignore-test-list strs) > + (map (lambda (str) > + (let ((ignore-string (format #f "#[ignore]\n~a" str))) > + `((,str) ,ignore-string))) > + strs)) A few things here. Firstly, I'm not sure that this procedure should exist. Ideally we'd like to fix rather than ignore as many tests as possible - although I see you're using this to rewrite existing stuff more cleanly. Either way, break this whole function and its usage out into a separate patch. Move it to be near other helper functions, not in the middle of package definitions. Also perhaps make it a bit cleaner like: > (define (ignore-rust-tests strs) > (map (lambda (str) > `((,str) ,(string-append "#[ignore]\n" str)) > strs)) It's worth noting this is essentially a non-hygienic macro; maybe look into rewriting it as a macro instead. See https://spritely.institute/static/papers/scheme-primer.html#scheme-extensibility for an introduction to the ideas behind macros, and https://www.gnu.org/software/guile/manual/html_node/Macros.html - especially https://www.gnu.org/software/guile/manual/html_node/Syntax-Rules.html. I've also found the (incorrectly) linked and very excellent http://www.phyast.pitt.edu/~micheles/syntax-rules.pdf (WARNING! IS A PDF! also available from https://gist.github.com/jgarte/beb03e000943b7426f00b3d04ed01262 (WARNING! IS GITHUB!)) to be incredibly helpful. Ideally instead of using `(substitutes* ...)` you'd simply have some thing like `(ignore-rust-tests '( ...))` which produces the right code. Note that if you rewrite this as a macro, you won't be able to use docstrings. > + ;; Gitoxide tests seem to require the internet to run > + ;; and Guix build containers don't have the internet. You can just say, "Gitoxide tests require the network" or similar. > - (substitute* > - "src/tools/cargo/tests/testsuite/init/simple_hg_ignore_exists/mod.rs" > - (("fn simple_hg_ignore_exists") > - "#[ignore]\nfn simple_hg_ignore_exists")) > (substitute* > "src/tools/cargo/tests/testsuite/init/mercurial_autodetect/mod.rs" > - (("fn mercurial_autodetect") > - "#[ignore]\nfn mercurial_autodetect")))) > + ,@(make-ignore-test-list '("fn case"))) > + (substitute* > + "src/tools/cargo/tests/testsuite/init/simple_hg/mod.rs" > + ,@(make-ignore-test-list '("fn case"))) > + (substitute* > + "src/tools/cargo/tests/testsuite/init/simple_hg_ignore_exists/mod.rs" > + ,@(make-ignore-test-list '("fn case"))) In this code, you move down the substitute* call on simple_hg_ignore_exists/mod.rs and add code above it. This produces a larger diff than necessary. Avoid changing code order like this without a good reason. > + ;; Append the default output's lib folder to the RUSTFLAGS > + ;; environment variable. this lets programs that depend on > + ;; rustc's shared libraries like rustfmt work. Minor writing stuff. Capitalize the 't' in "this;" move "like rustfmt" to after "programs." > + (setenv "RUSTFLAGS" > + (format #f "-C link-arg=-Wl,-rpath,~a/lib" > + (assoc-ref outputs "out"))) I'm not sure that this is the best way to do this. Setting build flags in an environment variable just feels wrong. I'll let someone with more knowledge of the domain weigh in, but I do want to mention it. I don't think any of the rest of this review is blocking, but you may want to consider it anyway. I don't see "format" used in package definitions very often; I don't know if there's some reason to oppose it, but it's worth considering just using "string-append" instead. Make sure this builds on architectures besides x86_64 if you can. I see in the commit history for Rust there seem to be some issues around aarch64 and riscv64 support in particular, so testing for those platforms may reveal more issues. Make sure to run "guix style" and "guix lint". You may want to have a separate patch, either before or after your changes, to port all of the default rust package's modify-phases to g-expressions; "guix style -S arguments" should help there. You'll need to wait for review from experts on the subject, and obviously someone with commit, but this is very impressive for a first patch. Rust has been languishing for a while and it's clearly not just because it's not used. Well done. - Juli