From: Juliana Sims <juli@incana.org>
To: 64804@debbugs.gnu.org
Cc: fries1234@protonmail.com
Subject: [bug#64804] [PATCH] gnu: rust: Update to Rust 1.71.0
Date: Sun, 23 Jul 2023 16:38:10 -0400 [thread overview]
Message-ID: <MZN9YR.MV79N2FVWSJD@incana.org> (raw)
In-Reply-To: <1bbbeed9c7c6e50464bee042e8dd06e9cd62c4c2.1690094273.git.fries1234@protonmail.com>
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* <file> ...)` you'd simply have some thing like
`(ignore-rust-tests <file> '(<str> <strs> ...))` 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
next prev parent reply other threads:[~2023-07-23 20:39 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-23 6:38 [bug#64804] [PATCH] gnu: rust: Update to Rust 1.71.0 Fries via Guix-patches via
2023-07-23 20:38 ` Juliana Sims [this message]
2023-07-24 5:36 ` [bug#64804] [PATCH 1/5] gnu: rust: Add rust-1.69 Fries via Guix-patches via
2023-07-24 5:36 ` [bug#64804] [PATCH 2/5] gnu: rust: Add rust-1.70 Fries via Guix-patches via
2023-07-24 5:36 ` [bug#64804] [PATCH 3/5] gnu: rust: Add rust-1.71 Fries via Guix-patches via
2023-07-24 5:36 ` [bug#64804] [PATCH 4/5] gnu: rust: Add make-ignore-test-list function Fries via Guix-patches via
2023-07-24 5:36 ` [bug#64804] [PATCH 5/5] gnu: rust: Update to 1.71 Fries via Guix-patches via
2023-07-24 5:51 ` Fries via Guix-patches via
2023-10-01 7:10 ` Efraim Flashner
2023-09-30 15:40 ` [bug#64804] Please merge it Milan Svoboda
2023-10-01 9:30 ` Malte Frank Gerdes
2023-10-01 3:47 ` [bug#64804] [PATCH] gnu: rust: Update to Rust 1.71.0 jaeme via Guix-patches via
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=MZN9YR.MV79N2FVWSJD@incana.org \
--to=juli@incana.org \
--cc=1bbbeed9c7c6e50464bee042e8dd06e9cd62c4c2.1690094273.git.fries1234@protonmail.com \
--cc=64804@debbugs.gnu.org \
--cc=fries1234@protonmail.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.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/guix.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.