unofficial mirror of guix-patches@gnu.org 
 help / color / mirror / code / Atom feed
From: Maxime Devos <maximedevos@telenet.be>
To: Dion Mendel <guix@dm9.info>, 47921@debbugs.gnu.org
Subject: [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname.
Date: Wed, 21 Apr 2021 20:33:57 +0200	[thread overview]
Message-ID: <92c4f265369735c71c6f358cfee29fb7cc6a38d2.camel@telenet.be> (raw)
In-Reply-To: <20210421154612.GA956@dm9.info>

[-- Attachment #1: Type: text/plain, Size: 5004 bytes --]

Dion Mendel schreef op wo 21-04-2021 om 23:46 [+0800]:
> Thanks for the follow up.
> 
> > 1. IIUC, this change would cause a world-rebuild, so this patch would 
> > have to applied on core-updates.  The subject line should have been
> > [PATCH core-updates]: etcetera.
> 
> Are you sure?  This patch will modify guix.
> 
>    guix graph --type=reverse-package guix
>
> Only shows a few packages to be rebuilt.  I am new to guix so I may be 
> wrong about this.

If this patch modified something in, say, guix/scripts or gnu/packages,
targetting "master" should be perfectly fine.  But this patch modifies
something under guix/build.  Modules under guix/build can be used from
the build environment, that is, from within the compilation process of
a package.  By modifying guix/build/gremlin.scm, a widely-used module
in package definitions (used indirectly via gnu-build-system IIRC),
practically all packages would be rebuilt.

Unless I'm severely mistaken, you can see this for yourself by:

0. start from a checkout of guix *without* your patch
1. # you probably have its dependencies already, but let's make sure
   ./pre-inst-env guix build hello
2. apply your patch to your local checkout of guix
3. make
4. ./pre-inst-env guix build hello
   # This will probably rebuild GCC, binutils, etc.!

> 2. How did you test this patch?
> 
> Tested in the repl.
> 
> Current behaviour:
> 
>     (elf-dynamic-info-soname
>       (call-with-input-file "/path/to/libz.so.1.2.11"
>                             (compose elf-dynamic-info parse-elf
>                                      get-bytevector-all)))
>     => #<<dynamic-entry> type: 14 value: "libz.so.1" offset: 5764>
> 
> There is no way to extract the value as dynamic-entry is private to the 
> module.
> 
> After the patch:
> 
>     (elf-dynamic-info-soname
>       (call-with-input-file "/path/to/libz.so.1.2.11"
>                             (compose elf-dynamic-info parse-elf
>                                      get-bytevector-all)))
>     => "libz.so.1"

So you didn't try to build any package with this patch?

I would recommend to make sure this patch doesn't break any packages.
While you can't test all packages (unless you have a *really big* build
farm), I would at least suggest running "./pre-inst-env guix build hello".
If that takes too long to complete, don't worry, just interrupt it and let
us now you couldn't test it completely.

To make sure this new functionality does not break in the future, please
write an unit test (in tests/gremlin.scm).

> > 3. What does this patch fix?
> 
> Module (guix build gremlin) exports several functions to extract 
> information from the dynamic section of an elf file.
> 
> elf-dynamic-info-soname is one of these functions.  It is not called 
> anywhere in guix.  I would like to use it for packaging, but it is 
> currently non functioning.

Your patch doesn't modify elf-dynamic-info-soname.  It modifies elf-dynamic-info.

> > 4. elf-dynamic-info-soname is a record accessor.  Did you mean 
> > elf-dynamic-info?

In case I wasn't clear, I was referring to the commit message.  In the commit
message, you say you modified elf-dynamic-info-soname, but you actually modified
elf-dynamic-info.
 
> No, I do not mean elf-dynamic-info.
See two comments above.

> elf-dynamic-info-soname is a record accessor which is currently broken 
Record accessors are correct by construction.  Perhaps you meant
that the "soname" field is initialised incorrectly by elf-dynamic-info?

> because it doesn't unwrap an internal structure, namely <dynamic-entry>.  
> All the other accessors unwrap this internal structure.
I think you can predict my response about accessors here (-:.

I'll interpret this as ‘in all other fields, the internal structure is unwrapped’.
That's a good point!  Your patch seems good to me, but the commit message doesn't
and it has a lack of testing.

> This patch brings this accessor into line with the others.

You didn't modify the elf-dynamic-info-soname, you modified elf-dynamic-info.
See comments above.

> > 5. According to the docstring (core-updates, c9a61dff8242612ae8275829a5ee31ff45ff08b1):
> > 
> >  "Return dynamic-link information for ELF as an <elf-dynamic-info> object, or
> >  #f if ELF lacks dynamic-link information."
> > 
> >  So this patch actually _introduces_ a bug.  Or you need to modify the docstring
> >  as well.

On second thought, the (revised) patch actually seems correct, and my comment here
doesn't make much sense.

> No.  This patch does not affect elf-dynamic-info.  It fixes one of its 
> accessors.

You modified elf-dynamic-info, so this patch does affect it.
elf-dynamic-info is a procedure, so it cannot have any accessors (unless you play
weird tricks with Guile's undocumented ‘applicable structs’).  The record is
<elf-dynamic-info>. <elf-dynamic-info> != elf-dynamic-info.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

  reply	other threads:[~2021-04-21 18:43 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-20 19:44 [bug#47921] [PATCH] build: Fix elf-dynamic-info-soname Dion Mendel
2021-04-21  2:46 ` Dion Mendel
2021-04-21 11:49 ` Maxime Devos
2021-04-21 15:46   ` Dion Mendel
2021-04-21 18:33     ` Maxime Devos [this message]
2021-04-22  3:13       ` Dion Mendel
2021-04-22  8:20         ` bug#47921: " Ludovic Courtès
2021-04-22  2:55 ` [bug#47921] retitle Dion Mendel
2021-04-22  3:26 ` [bug#47921] Include correct commit message Dion Mendel

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

  List information: https://guix.gnu.org/

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

  git send-email \
    --in-reply-to=92c4f265369735c71c6f358cfee29fb7cc6a38d2.camel@telenet.be \
    --to=maximedevos@telenet.be \
    --cc=47921@debbugs.gnu.org \
    --cc=guix@dm9.info \
    /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 public inbox

	https://git.savannah.gnu.org/cgit/guix.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).