mbox series

[bug#49672,0/9] Target check fixes and cleanups

Message ID 20210721004615.631777-1-bauermann@kolabnow.com
Headers show
Series Target check fixes and cleanups | expand

Message

Thiago Jung Bauermann July 21, 2021, 12:46 a.m. UTC 
Hello,

I recently had a closer look at uses of ‘%current-target-system’ and
‘%current-system’ and noticed a few bugs to fix and also cleanups that can
be done.

They mostly stem from the fact that ‘%current-system’ holds a Nix system
identifier, while ‘%current-target-system’ holds a GNU triplet identifier.
A few parts of the code are confused about that and make the wrong check.
The individual patch descriptions have the details.

The last two patches are just cleanups and not bug fixes. I’m on the fence
about the merits of the last patch. It can reasonably be seen as either a
worthwhile cleanup or needless code churn. I’m sending it anyway, and I’m
fine with either dropping or applying it.

The series applies on today’s master branch, as of commit:

e644e45aec26 gnu: pipe-viewer: Correct references to youtube-dl program.

The only testing I did was running `make` on each patch in the series and
checking if there’s  any new Guile warnings or errors (there isn’t), and
running `make check`. The testsuite results are:

============================================================================
Testsuite summary for GNU Guix UNKNOWN
============================================================================
# TOTAL: 1935
# PASS:  1903
# SKIP:  27
# XFAIL: 2
# FAIL:  3
# XPASS: 0
# ERROR: 0
============================================================================
See ./test-suite.log
Please report to bug-guix@gnu.org
============================================================================

The failures are in tests/publish.scm, tests/guix-describe.sh and
tests/guix-package.sh. Compared to the master branch,
tests/guix-describe.sh is the only new failure. The test is skipped in the
master branch. This is because I ran the master branch tests on a git
worktree, where ‘.git’ is a file rather than a directory, causing the test
to be skipped. If I change the `if [ -d "…/.git" ]` line to
`if [ -e "…/.git" ]`, then the test also fails on master and I have the same
testsuite results with and without the patches.

Thiago Jung Bauermann (9):
  gnu: Fix check of ‘%current-system’ and ‘%current-target-system’
  utils: Add ‘current-target-nix-system’ helper
  gnu: Query membership in ‘supported-systems’ list with Nix identifier
  gnu: Use ‘current-target-nix-system’ in pattern matches
  gnu: Use ‘current-target-nix-system’ in prefix checks
  gnu: Fix GNU/Linux system detection
  bootloader: grub: Fix mix of GNU triplet and Nix system identifier
  gnu: ustr: Don’t pass default argument
  gnu: Use existing target helpers from ‘(guix utils)’

 gnu/bootloader/grub.scm         |  5 +----
 gnu/packages/algebra.scm        |  3 +--
 gnu/packages/astronomy.scm      |  4 ++--
 gnu/packages/bdw-gc.scm         |  4 ++--
 gnu/packages/boost.scm          |  4 ++--
 gnu/packages/bootloaders.scm    | 22 ++++++----------------
 gnu/packages/bootstrap.scm      |  6 ++----
 gnu/packages/c.scm              |  3 +--
 gnu/packages/ci.scm             |  6 ++----
 gnu/packages/compression.scm    |  6 ++----
 gnu/packages/crypto.scm         |  4 ++--
 gnu/packages/databases.scm      |  3 +--
 gnu/packages/debug.scm          |  6 ++----
 gnu/packages/digest.scm         |  3 +--
 gnu/packages/firmware.scm       |  4 ++--
 gnu/packages/gcc.scm            |  5 +----
 gnu/packages/golang.scm         |  6 ++----
 gnu/packages/gstreamer.scm      |  6 ++----
 gnu/packages/java.scm           | 16 +++++++---------
 gnu/packages/julia.scm          |  3 +--
 gnu/packages/linux.scm          |  4 +---
 gnu/packages/lisp.scm           |  3 +--
 gnu/packages/llvm.scm           |  4 +---
 gnu/packages/make-bootstrap.scm |  2 +-
 gnu/packages/mes.scm            |  3 +--
 gnu/packages/messaging.scm      | 12 ++++++------
 gnu/packages/multiprecision.scm |  4 +---
 gnu/packages/ntp.scm            |  4 ++--
 gnu/packages/pascal.scm         |  6 ++----
 gnu/packages/pciutils.scm       |  3 +--
 gnu/packages/photo.scm          |  4 ++--
 gnu/packages/php.scm            |  3 +--
 gnu/packages/python.scm         | 26 ++++++++++----------------
 gnu/packages/rust.scm           |  4 +---
 gnu/packages/sdl.scm            |  4 ++--
 gnu/packages/syncthing.scm      |  4 ++--
 gnu/packages/telephony.scm      |  4 ++--
 gnu/packages/textutils.scm      |  3 +--
 gnu/packages/valgrind.scm       |  4 ++--
 gnu/packages/video.scm          |  7 ++-----
 gnu/packages/web.scm            |  9 +++------
 guix/utils.scm                  | 14 ++++++++++++++
 42 files changed, 101 insertions(+), 149 deletions(-)

Comments

M Nov. 7, 2021, 11:23 a.m. UTC | #1 
Hi,

I looked over the patch series, and almost all seems reasonable to me.
though I would split patch 7/9 into a patch defining
current-target-gnu-triplet and into a patch using current-target-gnu-
triplet,
and possibly a rebase might be neccesary.

Warning: I only looked at the patches, I didn't apply them for testing.

I don't think the following change is necessary though:

-           ,@(if (string-match "^x86_64-linux"
-                               (or (%current-target-system)
-                                   (%current-system)))
+           ,@(if (string-match "^x86_64-linux" (current-target-nix-
system))

because the regex ^x86_64-linux would match both the nix system and GNU
triplet
IIUC.

Greetings,
Maxime.
Thiago Jung Bauermann Nov. 12, 2021, 12:56 a.m. UTC | #2 
Hello Maxime,

Em domingo, 7 de novembro de 2021, às 08:23:37 -03, Maxime Devos escreveu:
> I looked over the patch series, and almost all seems reasonable to me.

Thank you for reviewing the patches! I will work on a second version 
addressing your comments.

> though I would split patch 7/9 into a patch defining
> current-target-gnu-triplet and into a patch using current-target-gnu-
> triplet,

Ok, I will do this.

> and possibly a rebase might be neccesary.

I will do this as well.

> Warning: I only looked at the patches, I didn't apply them for testing.

I don’t recall how thoroughly I tested these patches. I’ll try to do more 
extensive testing on v2.

> I don't think the following change is necessary though:
> 
> -           ,@(if (string-match "^x86_64-linux"
> -                               (or (%current-target-system)
> -                                   (%current-system)))
> +           ,@(if (string-match "^x86_64-linux" (current-target-nix-
> system))
> 
> because the regex ^x86_64-linux would match both the nix system and GNU
> triplet
> IIUC.

Maybe in the way Guix uses GNU triplets the regex would match (I don’t 
actually know) but in theory at least it wouldn’t, because “x86_64-linux” 
isn’t a complete GNU triplet: it’s missing the vendor field. A complete 
triplet would be “x86_64-unknown-linux-gnu”, or “x86_64-pc-linux-gnu”.

There’s a bit more information here: https://wiki.osdev.org/Target_Triplet

This is actually one of the main reasons why I decided to clean up the 
handling of Nix system identifiers and GNU triplet identifiers.