inkscape retains a reference to imagemagick, even though it is in native-inputs

Done

Details

5 participants

Efraim Flashner
Leo Famulari
Maxim Cournoyer
Maxime Devos
Mark H Weaver

Owner: unassigned

Submitted by: Maxime Devos

Severity: normal

Maxime Devos wrote on 29 Mar 2021 18:33

Recipients:(address . bug-guix@gnu.org)

Message-ID:9beb8d6af78a517d53aaaa43179272b8953da78f.camel@telenet.be

Hi Guix,

$ guix --version

Toggle quote (2 lines)

> guix (GNU Guix) 510e24f973a918391d8122fd6ad515c0567bf23e

with

$ guix graph --type=references inkscape

it can be seen inkscape retains the reference to imagemagick,

even though imagemagick is in native-inputs.

Greetings,

Maxime.

-----BEGIN PGP SIGNATURE-----

iI0EABYIADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYGIBIBccbWF4aW1lZGV2

b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7kloAQDcUuMR0SV1+0IlP3Mt2qc2ww7C

oJMw5ZYQmWZ+vXekVwEAwc6c/NlyGkPi10VpSdjkpZHC9Qh+k3Dx9c5uWWCBCg0=

=Kdnx

-----END PGP SIGNATURE-----

Mark H Weaver wrote on 29 Mar 2021 23:16

Recipients:

Message-ID:87lfa53bkj.fsf@netris.org

Hi Maxime,

Maxime Devos <maximedevos@telenet.be> writes:

Toggle quote (10 lines)> On
> $ guix --version
>> guix (GNU Guix) 510e24f973a918391d8122fd6ad515c0567bf23e
>
> with
> $ guix graph --type=references inkscape
>
> it can be seen inkscape retains the reference to imagemagick,
> even though imagemagick is in native-inputs.

I believe this is incorrect. On my Guix system, I see this:

Toggle snippet (5 lines)

mhw@jojen ~$ guix package -I inkscape

inkscape 0.92.4 out /gnu/store/rx26f9xn14fng2gnzsigr74492lrkydl-inkscape-0.92.4

mhw@jojen ~$ guix gc -R /gnu/store/rx26f9xn14fng2gnzsigr74492lrkydl-inkscape-0.92.4 | grep -i imagemagick

I guess that "guix graph --type=references" is not a reliable indicator

of what we need to check for.

FYI, this is what I do to check that my system and user profile do not

have references to imagemagick:

Toggle snippet (4 lines)

mhw@jojen ~$ guix gc -R $(readlink /run/current-system) | grep -i imagemagick

mhw@jojen ~$ guix gc -R $(readlink -f ~/.guix-profile) | grep -i imagemagick

Regards,

Mark

Efraim Flashner wrote on 30 Mar 2021 09:19

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:YGLRBGytQfTcioO4@3900XT

On Mon, Mar 29, 2021 at 05:16:01PM -0400, Mark H Weaver wrote:

Toggle quote (22 lines)> Hi Maxime,
> 
> Maxime Devos <maximedevos@telenet.be> writes:
> 
> > On
> > $ guix --version
> >> guix (GNU Guix) 510e24f973a918391d8122fd6ad515c0567bf23e
> >
> > with
> > $ guix graph --type=references inkscape
> >
> > it can be seen inkscape retains the reference to imagemagick,
> > even though imagemagick is in native-inputs.
> 
> I believe this is incorrect.  On my Guix system, I see this:
> 
> --8<---------------cut here---------------start------------->8---
> mhw@jojen ~$ guix package -I inkscape
> inkscape	0.92.4	out	/gnu/store/rx26f9xn14fng2gnzsigr74492lrkydl-inkscape-0.92.4
> mhw@jojen ~$ guix gc -R /gnu/store/rx26f9xn14fng2gnzsigr74492lrkydl-inkscape-0.92.4 | grep -i imagemagick
> --8<---------------cut here---------------end--------------->8---

It is the case for inkscape@1.0.2

(ins)efraim@3900XT ~$ guix package -A inkscape

inkscape 1.0.2 out gnu/packages/inkscape.scm:121:2

inkscape 0.92.4 out gnu/packages/inkscape.scm:56:2

(ins)efraim@3900XT ~$ guix gc --references $(guix build inkscape@1) | grep -i imagemagick

/gnu/store/y4rym48vihmh3jk9qnll0zqfnxzi9n6v-imagemagick-6.9.12-4

I believe it comes from the glib-or-gtk-wrap phase where it wraps the

XDG_DATA_DIRS and likely grabs more than it needs.

Toggle quote (17 lines)> I guess that "guix graph --type=references" is not a reliable indicator
> of what we need to check for.
> 
> FYI, this is what I do to check that my system and user profile do not
> have references to imagemagick:
> 
> --8<---------------cut here---------------start------------->8---
> mhw@jojen ~$ guix gc -R $(readlink /run/current-system) | grep -i imagemagick
> mhw@jojen ~$ guix gc -R $(readlink -f ~/.guix-profile) | grep -i imagemagick
> --8<---------------cut here---------------end--------------->8---
> 
>     Regards,
>       Mark
> 
> 
> 

-- 
Efraim Flashner   <efraim@flashner.co.il>   ????? ?????
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

Mark H Weaver wrote on 30 Mar 2021 10:55

Recipients:(name . Efraim Flashner)(address . efraim@flashner.co.il)

Message-ID:8735wd2f77.fsf@netris.org

Hi Efraim,

Efraim Flashner <efraim@flashner.co.il> writes:

Toggle quote (2 lines)

> It is the case for inkscape@1.0.2

I see now that I'm using an older version, although I would have

preferred the newer one. I refer to the variable name 'inkscape' from

my manifest file, and I expected that to point to the latest stable

version. However, it seems that one must use the 'inkscape-1.0'

variable to get the latest stable version. That's seems suboptimal.

I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'

(for use in packages such as 'dblatex/stable'), and then 'inkscape'

could be repurposed to point to the latest stable version. Thoughts?

Toggle quote (9 lines)

> (ins)efraim@3900XT ~$ guix package -A inkscape

> inkscape 1.0.2 out gnu/packages/inkscape.scm:121:2

> inkscape 0.92.4 out gnu/packages/inkscape.scm:56:2

> (ins)efraim@3900XT ~$ guix gc --references $(guix build inkscape@1) | grep -i imagemagick

> /gnu/store/y4rym48vihmh3jk9qnll0zqfnxzi9n6v-imagemagick-6.9.12-4

> I believe it comes from the glib-or-gtk-wrap phase where it wraps the

> XDG_DATA_DIRS and likely grabs more than it needs.

Good catch!

So, for now, we shouldn't use 'imagemagick/stable' in 'inkscape', even

though it's in 'native-inputs'. This shows that we'll have to be very

careful about this, at least until we have better tooling to catch these

problems automatically.

I think the fundamental problem here is that the build-side code cannot

distinguish between 'inputs' and 'native-inputs' unless you are

cross-compiling. When compiling natively, the 'inputs' keyword argument

passed to the build phases includes the packages listed in the

'native-inputs' package field, and the 'native-inputs' keyword argument

is not passed at all.

This is why we must write (or native-inputs inputs) in so many places:

because to find the location of a package listed in the 'native-inputs'

package field from within build-side code, you must look in one of two

places depending on whether you're cross-compiling. If you're

cross-compiling you must look in 'native-inputs'. When compiling

natively, that argument doesn't even exist, and you must look in the

'inputs' keyword argument instead.

I don't know why it was done this way. It seems to me an error-prone

design, but at this point it would be hard to change it.

Now we see another disadvantage. The 'glib-or-gtk-wrap' phase should be

iterating over the 'inputs' but not the 'native-inputs'. It's not

obvious how to fix this given the current design.

What do you think?

Regards,

Mark

Efraim Flashner wrote on 30 Mar 2021 14:02

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:YGMTZLMggCOVn7aU@3900XT

On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:

Toggle quote (15 lines)> Hi Efraim,
> 
> Efraim Flashner <efraim@flashner.co.il> writes:
> > It is the case for inkscape@1.0.2
> 
> I see now that I'm using an older version, although I would have
> preferred the newer one.  I refer to the variable name 'inkscape' from
> my manifest file, and I expected that to point to the latest stable
> version.  However, it seems that one must use the 'inkscape-1.0'
> variable to get the latest stable version.  That's seems suboptimal.
> 
> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'
> (for use in packages such as 'dblatex/stable'), and then 'inkscape'
> could be repurposed to point to the latest stable version.  Thoughts?

In the past we've kept the most up-to-date version as the 'main version'

and given the version suffix to the older version(s). Except for when

they all get version suffixes and then a separate (define rust

rust-1.45) package added.

Toggle quote (24 lines)> 
> > (ins)efraim@3900XT ~$ guix package -A inkscape
> > inkscape        1.0.2   out     gnu/packages/inkscape.scm:121:2
> > inkscape        0.92.4  out     gnu/packages/inkscape.scm:56:2
> > (ins)efraim@3900XT ~$ guix gc --references $(guix build inkscape@1) | grep -i imagemagick
> > /gnu/store/y4rym48vihmh3jk9qnll0zqfnxzi9n6v-imagemagick-6.9.12-4
> >
> > I believe it comes from the glib-or-gtk-wrap phase where it wraps the
> > XDG_DATA_DIRS and likely grabs more than it needs.
> 
> Good catch!
> 
> So, for now, we shouldn't use 'imagemagick/stable' in 'inkscape', even
> though it's in 'native-inputs'.  This shows that we'll have to be very
> careful about this, at least until we have better tooling to catch these
> problems automatically.
> 
> I think the fundamental problem here is that the build-side code cannot
> distinguish between 'inputs' and 'native-inputs' unless you are
> cross-compiling.  When compiling natively, the 'inputs' keyword argument
> passed to the build phases includes the packages listed in the
> 'native-inputs' package field, and the 'native-inputs' keyword argument
> is not passed at all.

I ran into this also with the cargo-build-system. I only wanted to

propagate regular inputs, not native inputs. It's probably worth

figuring out how to fix some of it on core-updates.

Toggle quote (17 lines)> This is why we must write (or native-inputs inputs) in so many places:
> because to find the location of a package listed in the 'native-inputs'
> package field from within build-side code, you must look in one of two
> places depending on whether you're cross-compiling.  If you're
> cross-compiling you must look in 'native-inputs'.  When compiling
> natively, that argument doesn't even exist, and you must look in the
> 'inputs' keyword argument instead.
> 
> I don't know why it was done this way.  It seems to me an error-prone
> design, but at this point it would be hard to change it.
> 
> Now we see another disadvantage.  The 'glib-or-gtk-wrap' phase should be
> iterating over the 'inputs' but not the 'native-inputs'.  It's not
> obvious how to fix this given the current design.
> 
> What do you think?

We can always try to make it better. In the mean time perhaps we can try

changing the way some of the wrappers work to only accept regular

inputs, or possibly to specify exactly which inputs to iterate over and

to use in the wrap phases.

Toggle quote (3 lines)

> Regards,

> Mark

-- 
Efraim Flashner   <efraim@flashner.co.il>   ????? ?????
GPG key = A28B F40C 3E55 1372 662D  14F7 41AA E7DC CA3D 8351
Confidentiality cannot be guaranteed on emails sent or received unencrypted

Leo Famulari wrote on 30 Mar 2021 17:38

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:YGNF9O2KS+7hBwnV@jasmine.lan

On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:

Toggle quote (10 lines)> I see now that I'm using an older version, although I would have
> preferred the newer one.  I refer to the variable name 'inkscape' from
> my manifest file, and I expected that to point to the latest stable
> version.  However, it seems that one must use the 'inkscape-1.0'
> variable to get the latest stable version.  That's seems suboptimal.
> 
> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'
> (for use in packages such as 'dblatex/stable'), and then 'inkscape'
> could be repurposed to point to the latest stable version.  Thoughts?

I think we should do this, or even remove the old Inkscape package now.

I'm guessing the reason for keeping the older release series is that

the Inkscape save-file format changed?

By the way, there is a new upstream release of the old series available,

0.92.5.

Mark H Weaver wrote on 31 Mar 2021 00:48

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:87wnto1cms.fsf@netris.org

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

Toggle quote (7 lines)

> On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:

>> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'

>> (for use in packages such as 'dblatex/stable'), and then 'inkscape'

>> could be repurposed to point to the latest stable version. Thoughts?

> I think we should do this,

Thanks.

Toggle quote (2 lines)

> or even remove the old Inkscape package now.

I don't think we can remove the old Inkscape, because 'inkscape' is an

input of 'dblatex/stable', which 'gtk-doc/stable' depends on.

There might or might not be other reasons to keep an older version of

'inkscape' around, but either way, if 'gtk-doc/stable' depends on

Inkscape, I think we'd better have an 'inkscape/stable' too, so that our

'inkscape' package can be freely updated on 'master'.

Going forward, if it turns out that Inkscape is not truly needed as an

input to 'dblatex/stable', and moreover, if _nothing_ deep in our stack

truly needs to depend on Inkscape, then we could perhaps eliminate

'inkscape/stable' altogether. However, that would need to happen on

either 'core-updates' or 'staging' (or similar), because changing

'dblatex/stable' entails too many rebuilds.

What do you think?

Thanks,

Mark

Leo Famulari wrote on 31 Mar 2021 07:30

Recipients:(name . Mark H Weaver)(address . mhw@netris.org)

Message-ID:YGQJAOETA3+YleQT@jasmine.lan

On Tue, Mar 30, 2021 at 06:48:16PM -0400, Mark H Weaver wrote:

Toggle quote (17 lines)> I don't think we can remove the old Inkscape, because 'inkscape' is an
> input of 'dblatex/stable', which 'gtk-doc/stable' depends on.
> 
> There might or might not be other reasons to keep an older version of
> 'inkscape' around, but either way, if 'gtk-doc/stable' depends on
> Inkscape, I think we'd better have an 'inkscape/stable' too, so that our
> 'inkscape' package can be freely updated on 'master'.
> 
> Going forward, if it turns out that Inkscape is not truly needed as an
> input to 'dblatex/stable', and moreover, if _nothing_ deep in our stack
> truly needs to depend on Inkscape, then we could perhaps eliminate
> 'inkscape/stable' altogether.  However, that would need to happen on
> either 'core-updates' or 'staging' (or similar), because changing
> 'dblatex/stable' entails too many rebuilds.
> 
> What do you think?

I didn't realize / remember that Inkscape was used that deep in the
package graph. I agree, we should delay this change, at least until a
rebuild cycle.

I do think it's suboptimal that an end-user application like Inkscape
is depended on by so many packages...

Mark H Weaver wrote on 31 Mar 2021 09:52

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:871rbv220e.fsf@netris.org

Hi Leo,

Leo Famulari <leo@famulari.name> writes:

Toggle quote (4 lines)

> I didn't realize / remember that Inkscape was used that deep in the

> package graph. I agree, we should delay this change, at least until a

> rebuild cycle.

The removal of inkscape@0.92.4 should certainly be delayed, but I see no
reason why we couldn't immediately, on 'master', rename the variable
'inkscape' to 'inkscape/stable', and 'inkscape-1.0' to 'inkscape', with
'inkscape-1.0' made an alias to 'inkscape', if we can agree on it.
Do you see a reason to delay those changes?

Toggle quote (3 lines)

> I do think it's suboptimal that an end-user application like Inkscape

> is depended on by so many packages...

Indeed, it's not good. In fact, the question just occurred to me:

"How is it that Inkscape, which clearly depends on Gtk+, can also be a

dependency of Gtk+, via the path gtk+ -> at-spi2-atk -> at-spi2-core

-> gtk-doc -> dblatex -> inkscape@0.92.4?"

It turns out that the only reason there's no cycle here is because:

(1) the older inkscape@0.92.4 uses gtk+-2 (not 3), and

(2) none of the dependencies of gtk+-2 use gtk-doc.

Both of these are likely suboptimal, but we will apparently be blocked

from fixing these issues while Inkscape is needed to build our core

graphics stack.

In my opinion, the best way to fix this is to split off documentation

generation for selected core libraries into separate packages.

Generating documentation often requires higher-level components, and yet

we should also generate documentation for our core libraries. This

naturally leads to cycles unless the documentation is split off. We

should use the core libraries (without docs) to build the documentation

generators, and then from there build the documentation for the core

libraries.

What do you think?

Thanks for the discussion,

Mark

Maxim Cournoyer wrote on 6 Apr 2021 16:15

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:87czv7pkic.fsf@gmail.com

Hi Leo et al.,

Leo Famulari <leo@famulari.name> writes:

Toggle quote (16 lines)> On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:
>> I see now that I'm using an older version, although I would have
>> preferred the newer one.  I refer to the variable name 'inkscape' from
>> my manifest file, and I expected that to point to the latest stable
>> version.  However, it seems that one must use the 'inkscape-1.0'
>> variable to get the latest stable version.  That's seems suboptimal.
>> 
>> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'
>> (for use in packages such as 'dblatex/stable'), and then 'inkscape'
>> could be repurposed to point to the latest stable version.  Thoughts?
>
> I think we should do this, or even remove the old Inkscape package now.
>
> I'm guessing the reason for keeping the older release series is that
> the Inkscape save-file format changed?

The reason inksape@0.92 is still kept around is becauseInkscape@1
doesn't build on ARM (more accurately, one of its dependencies,
lib2geom, doesn't).  It's been a while since I looked at the issue, and
it seems there may have been some activity in lib2geom upstream to try
to address the problem, so we should revisit it.

Maxim

Maxim Cournoyer wrote on 22 Jan 02:15 +0100

Recipients:(name . Leo Famulari)(address . leo@famulari.name)

Message-ID:87jzo26ueh.fsf@gmail.com

Hello,

Maxim Cournoyer <maxim.cournoyer@gmail.com> writes:

Toggle quote (26 lines)> Hi Leo et al.,
>
> Leo Famulari <leo@famulari.name> writes:
>
>> On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:
>>> I see now that I'm using an older version, although I would have
>>> preferred the newer one.  I refer to the variable name 'inkscape' from
>>> my manifest file, and I expected that to point to the latest stable
>>> version.  However, it seems that one must use the 'inkscape-1.0'
>>> variable to get the latest stable version.  That's seems suboptimal.
>>> 
>>> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'
>>> (for use in packages such as 'dblatex/stable'), and then 'inkscape'
>>> could be repurposed to point to the latest stable version.  Thoughts?
>>
>> I think we should do this, or even remove the old Inkscape package now.
>>
>> I'm guessing the reason for keeping the older release series is that
>> the Inkscape save-file format changed?
>
> The reason inksape@0.92 is still kept around is becauseInkscape@1
> doesn't build on ARM (more accurately, one of its dependencies,
> lib2geom, doesn't).  It's been a while since I looked at the issue, and
> it seems there may have been some activity in lib2geom upstream to try
> to address the problem, so we should revisit it.

That's not relevant anymore, but our current inkscape 1.2 depends on

imagemagick still. Seeing how it now links directly to it, I've added

it to inputs as well in commit 552ebc47af and pushed to core-updates.

Closing!

Thanks,

Maxim

Closed

Maxim Cournoyer wrote on 22 Jan 05:07 +0100

Recipients:

Message-ID:87cytu6mfd.fsf_-_@gmail.com

Hello,

[...]

Toggle quote (26 lines)>>> On Tue, Mar 30, 2021 at 04:55:13AM -0400, Mark H Weaver wrote:
>>>> I see now that I'm using an older version, although I would have
>>>> preferred the newer one.  I refer to the variable name 'inkscape' from
>>>> my manifest file, and I expected that to point to the latest stable
>>>> version.  However, it seems that one must use the 'inkscape-1.0'
>>>> variable to get the latest stable version.  That's seems suboptimal.
>>>> 
>>>> I wonder if the 'inkscape' variable should be renamed 'inkscape/stable'
>>>> (for use in packages such as 'dblatex/stable'), and then 'inkscape'
>>>> could be repurposed to point to the latest stable version.  Thoughts?
>>>
>>> I think we should do this, or even remove the old Inkscape package now.
>>>
>>> I'm guessing the reason for keeping the older release series is that
>>> the Inkscape save-file format changed?
>>
>> The reason inksape@0.92 is still kept around is becauseInkscape@1
>> doesn't build on ARM (more accurately, one of its dependencies,
>> lib2geom, doesn't).  It's been a while since I looked at the issue, and
>> it seems there may have been some activity in lib2geom upstream to try
>> to address the problem, so we should revisit it.
>
> That's not relevant anymore, but our current inkscape 1.2 depends on
> imagemagick still.  Seeing how it now links directly to it, I've added
> it to inputs as well in commit 552ebc47af and pushed to core-updates.

I've applied some patches from Maxime and refined my understanding of
what this was about; it was not just about retaining a reference to
imagemagick listed from native-inputs, it was about retaining a
reference to imagemagick for the *stable* variant of inkscape/stable,
which meant we couldn't use the imagemagick/stable insecure variant.

Tentatively fixed in b4a6b1ba93844d7373c58237cb0b742352dec954 ("gnu:
inkscape/stable: Build stable variant without imagemagick support.")
which builds on a series from Maxime Devos.  I haven't caught up with
rebuilding core-updates yet to validate it truly works, but we'll see
soon.

Thanks, Maxime!

-- 
Maxim

Closed

Your comment

This issue is archived.

To comment on this conversation send an email to 47479@debbugs.gnu.org

is:open	open issues
is:done	closed issues
submitter:<who>	search issue submitter
author:<who>	search by message author
date:yesterday..now	search by issue date
mdate:3m..2d	search by message date