Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On 31/07/2023 15.53, Michał Górny wrote:
On Mon, 2023-07-31 at 12:49 +0200, Florian Schmaus wrote:
On 31/07/2023 11.32, Sam James wrote:
Florian Schmaus writes:
[[PGP Signed Part:Undecided]]
On 31/07/2023 07.02, Michał Górny wrote:
On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote:
Which problem are we solving by moving away from this towards a slightly
more verbose construct?
The problem was that cargo.eclass ebuilds were taking significant
time
during cache regeneration and slowing down tools noticeably. No fancy
loops required, contrary to your great theory.
Removing the $()/fork from go-modules.eclass reduced the source time
of a package from 2400 milliseconds to 236 milliseconds.
Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris
reduces the source time from 44 milliseconds to 24 milliseconds.
That is a win in relative reduction, but absolute its just 20
milliseconds. Cache regeneration is an embarrassingly parallel
problem. Therefore such a reduction should not matter much, assuming
you have some parallelism on the hardware level.
Consistency matters
Sure, I would be in favor of consistently using $(foo_uris).
Especially since the performance gains of the variable-setting approach
are even lower than I first assumed. The cargo.eclass runs the function
that computes CARGO_CRATE_URIS now twice, which adds significantly more
overhead than the fork of $(foo_uris). See my patch to the ML.
So, to summarize, your point is that after you've ignored the original
thread
I can not say that I have actively ignored the thread. I am simply not
aware of such a thread where we discussed that the variable-setting
pattern has to be used instead of $(foo_uris).
I am sorry, I must have missed it. And it appears that my search-foo is
not strong enough to dig it up. Could you please point me to the thread?
Or, are you maybe referring to the thread with your cargo.eclass patches
from June, 16th? That seemed to be only about cargo.eclass and not about
a tree-wide policy.
and we've actually started switching stuff to ${xxx}, we should
reopen the discussion and start moving everything back to $(xxx),
I never demanded that anything should be moved back to $(foo_uris). It's
up to the eclass maintainer to decide which reasoning the follow and how
they weight the arguments.
even
though you've proven yourself that it's less optimal ("but only
a little!") and because... you prefer it?
Optimality is not one-dimensional. Speed is not everything.
The $(foo_uri) pattern has slightly less code complexity and is slightly
better readable. Moreover, given that the justification for moving away
from it is negligible speedup, I prefer $(foo_uri).
Moving from $(foo_uri) to the variable-setting pattern ${foo_uri} appear
to be a solution to a non-existing problem.
I have only expressed my opinion. I think that the variable-setting
pattern is somewhat disadvantageous in this case.
However, this is not a hill for me to die on.
Yes, that certainly makes
sense. It's surely a great way to run a distro is to undo optimizations
6 weeks later because you liked the old variant better.
Again, nobody asked for any undoing.
- Flow
OpenPGP_0x8CAC2A9678548E35.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On Mon, 2023-07-31 at 12:49 +0200, Florian Schmaus wrote:
> On 31/07/2023 11.32, Sam James wrote:
> >
> > Florian Schmaus writes:
> >
> > > [[PGP Signed Part:Undecided]]
> > > On 31/07/2023 07.02, Michał Górny wrote:
> > > > On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote:
> > > > > Which problem are we solving by moving away from this towards a
> > > > > slightly
> > > > > more verbose construct?
> > > > The problem was that cargo.eclass ebuilds were taking significant
> > > > time
> > > > during cache regeneration and slowing down tools noticeably. No fancy
> > > > loops required, contrary to your great theory.
> > >
> > > Removing the $()/fork from go-modules.eclass reduced the source time
> > > of a package from 2400 milliseconds to 236 milliseconds.
> > >
> > > Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris
> > > reduces the source time from 44 milliseconds to 24 milliseconds.
> > >
> > > That is a win in relative reduction, but absolute its just 20
> > > milliseconds. Cache regeneration is an embarrassingly parallel
> > > problem. Therefore such a reduction should not matter much, assuming
> > > you have some parallelism on the hardware level.
> >
> > Consistency matters
>
> Sure, I would be in favor of consistently using $(foo_uris).
>
> Especially since the performance gains of the variable-setting approach
> are even lower than I first assumed. The cargo.eclass runs the function
> that computes CARGO_CRATE_URIS now twice, which adds significantly more
> overhead than the fork of $(foo_uris). See my patch to the ML.
>
So, to summarize, your point is that after you've ignored the original
thread and we've actually started switching stuff to ${xxx}, we should
reopen the discussion and start moving everything back to $(xxx), even
though you've proven yourself that it's less optimal ("but only
a little!") and because... you prefer it? Yes, that certainly makes
sense. It's surely a great way to run a distro is to undo optimizations
6 weeks later because you liked the old variant better.
--
Best regards,
Michał Górny
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
Florian Schmaus writes: > [[PGP Signed Part:Undecided]] > On 31/07/2023 11.32, Sam James wrote: >> Florian Schmaus writes: >> >>> [[PGP Signed Part:Undecided]] >>> On 31/07/2023 07.02, Michał Górny wrote: On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote: > Which problem are we solving by moving away from this towards a slightly > more verbose construct? The problem was that cargo.eclass ebuilds were taking significant time during cache regeneration and slowing down tools noticeably. No fancy loops required, contrary to your great theory. >>> >>> Removing the $()/fork from go-modules.eclass reduced the source time >>> of a package from 2400 milliseconds to 236 milliseconds. >>> >>> Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris >>> reduces the source time from 44 milliseconds to 24 milliseconds. >>> >>> That is a win in relative reduction, but absolute its just 20 >>> milliseconds. Cache regeneration is an embarrassingly parallel >>> problem. Therefore such a reduction should not matter much, assuming >>> you have some parallelism on the hardware level. >> Consistency matters > > Sure, I would be in favor of consistently using $(foo_uris). Too late for that. Please don't reopen the cargo.eclass issue unnecessarily. > > Especially since the performance gains of the variable-setting > approach are even lower than I first assumed. The cargo.eclass runs > the function that computes CARGO_CRATE_URIS now twice, which adds > significantly more overhead than the fork of $(foo_uris). See my patch > to the ML. > >> and I already raised the point last week as well. > > Here on the mailing list, or somewhere else? > Here on the mailing list.
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On 31/07/2023 11.32, Sam James wrote: Florian Schmaus writes: [[PGP Signed Part:Undecided]] On 31/07/2023 07.02, Michał Górny wrote: On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote: Which problem are we solving by moving away from this towards a slightly more verbose construct? The problem was that cargo.eclass ebuilds were taking significant time during cache regeneration and slowing down tools noticeably. No fancy loops required, contrary to your great theory. Removing the $()/fork from go-modules.eclass reduced the source time of a package from 2400 milliseconds to 236 milliseconds. Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris reduces the source time from 44 milliseconds to 24 milliseconds. That is a win in relative reduction, but absolute its just 20 milliseconds. Cache regeneration is an embarrassingly parallel problem. Therefore such a reduction should not matter much, assuming you have some parallelism on the hardware level. Consistency matters Sure, I would be in favor of consistently using $(foo_uris). Especially since the performance gains of the variable-setting approach are even lower than I first assumed. The cargo.eclass runs the function that computes CARGO_CRATE_URIS now twice, which adds significantly more overhead than the fork of $(foo_uris). See my patch to the ML. > and I already raised the point last week as well. Here on the mailing list, or somewhere else? - Flow OpenPGP_0x8CAC2A9678548E35.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
Florian Schmaus writes: > [[PGP Signed Part:Undecided]] > On 31/07/2023 07.02, Michał Górny wrote: >> On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote: >>> Which problem are we solving by moving away from this towards a slightly >>> more verbose construct? >> The problem was that cargo.eclass ebuilds were taking significant >> time >> during cache regeneration and slowing down tools noticeably. No fancy >> loops required, contrary to your great theory. > > Removing the $()/fork from go-modules.eclass reduced the source time > of a package from 2400 milliseconds to 236 milliseconds. > > Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris > reduces the source time from 44 milliseconds to 24 milliseconds. > > That is a win in relative reduction, but absolute its just 20 > milliseconds. Cache regeneration is an embarrassingly parallel > problem. Therefore such a reduction should not matter much, assuming > you have some parallelism on the hardware level. Consistency matters and I already raised the point last week as well.
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On 31/07/2023 07.02, Michał Górny wrote: On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote: Which problem are we solving by moving away from this towards a slightly more verbose construct? The problem was that cargo.eclass ebuilds were taking significant time during cache regeneration and slowing down tools noticeably. No fancy loops required, contrary to your great theory. Removing the $()/fork from go-modules.eclass reduced the source time of a package from 2400 milliseconds to 236 milliseconds. Changing, for example net-p2p/arti-1.1.6, to use _cargo_set_crate_uris reduces the source time from 44 milliseconds to 24 milliseconds. That is a win in relative reduction, but absolute its just 20 milliseconds. Cache regeneration is an embarrassingly parallel problem. Therefore such a reduction should not matter much, assuming you have some parallelism on the hardware level. If switching to the variable-setting approach for something like cargo_crate_uris only motivated by reducing the cache regeneration a bit, then it is IMO borderline if its justified. - Flow OpenPGP_0x8CAC2A9678548E35.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On Sun, 2023-07-30 at 22:01 +0200, Maciej Barć wrote:
> > > +# @ECLASS_VARIABLE: NUGETS
> > > +# @DEFAULT_UNSET
> > > +# @PRE_INHERIT
> > > +# @DESCRIPTION:
> > > +# String containing all NuGet packages that need to be downloaded.
> > > +# Used by "nuget_uris".
> > > +#
> > > +# Example:
> > > +# @CODE
> > > +# NUGETS="
> > > +#ImGui.NET-1.87.2
> > > +#Config.Net-4.19.0
> > > +# "
> > > +#
> > > +# inherit dotnet-pkg
> > > +#
> > > +# ...
> > > +#
> > > +# SRC_URI+=" $(nuget_uris) "
> > > +# @CODE
> > > +
> > > +# @FUNCTION: nuget_uris
> > > +# @USAGE:
> > > +# @DESCRIPTION:
> > > +# Generates the URIs to put in SRC_URI to help fetch dependencies.
> > > +# If no arguments provided, uses the "NUGETS" variable.
> >
> > Sounds like you're copying the old, horribly slow cargo.eclass API.
> > Don't do that, unless you have to. Prefer variable-setting approach
> > used by modern cargo.eclass.
> >
>
> Please elaborate.
>
> Maybe such mechanisms could be extracted into standalone eclass? I do
> not know cargo, rust nor how it is used in Gentoo.
>
Calling subshells in global scope is a very bad idea, and it is
completely unnecessary in the most common case when NUGETS are defined
prior to inherit and URIs are inserted into top-level SRC_URI.
Just look at cargo.eclass.
> > > +# @FUNCTION: nuget_link
> > > +# @USAGE:
> > > +# @DESCRIPTION:
> > > +# Link a specified NuGet package at "nuget-path" to the "NUGET_PACKAGES"
> > > +# directory.
> > > +#
> > > +# Example:
> > > +# @CODE
> > > +# nuget_link "${DISTDIR}"/pkg.0.nupkg
> > > +# @CODE
> > > +#
> > > +# This function is used inside "dotnet-pkg_src_unpack"
> > > +# from the "dotnet-pkg" eclass.
> > > +nuget_link() {
> > > + [[ ! "${1}" ]] && die "${FUNCNAME}: no nuget path given"
> > > +
> > > + mkdir -p "${NUGET_PACKAGES}" || die
> > > +
> > > + local nuget_name="${1##*/}"
> > > +
> > > + if [[ -f "${NUGET_PACKAGES}"/${nuget_name} ]] ; then
> > > + ewarn "${FUNCNAME}: ${nuget_name} already exists"
> >
> > What does that mean? What is the user supposed to do about it? Is it
> > normal? If it's normal, then why are you warning about it? If it isn't
> > normal, then why isn't this fatal?
>
> This can happen if NuGets copied from SYSTEM_NUGETS are overwritten by
> ones specified in ebuild.
>
> This might be desired by ebuild author.
>
> We need to have this logged, I think ewarn is appropriate.
ewarn is a warning for the end user. The end user doesn't benefit from
being warned for "ebuild is doing something that might be wrong, or
might be perfectly fine, ignore this".
--
Best regards,
Michał Górny
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On Sun, 2023-07-30 at 22:19 +0200, Florian Schmaus wrote: > Which problem are we solving by moving away from this towards a slightly > more verbose construct? The problem was that cargo.eclass ebuilds were taking significant time during cache regeneration and slowing down tools noticeably. No fancy loops required, contrary to your great theory. -- Best regards, Michał Górny
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
On 30/07/2023 21.30, Michał Górny wrote:
On Sun, 2023-07-30 at 16:26 +0200, Maciej Barć wrote:
+# @FUNCTION: nuget_uris
+# @USAGE:
+# @DESCRIPTION:
+# Generates the URIs to put in SRC_URI to help fetch dependencies.
+# If no arguments provided, uses the "NUGETS" variable.
Sounds like you're copying the old, horribly slow cargo.eclass API.
Don't do that, unless you have to. Prefer variable-setting approach
used by modern cargo.eclass.
Should we make this a policy then?
On the other hand, I also wonder if its worth it.
You gave a similar comment when reviewing gradle.eclass and you changed
the cargo.eclass to also provide the "variable-setting approach" [1].
However, while the $() construct in shells is relatively expensive due
to the involved forking, it is usually only harmful when used in loops
with a high iteration count. The loop in go-modules.eclass was a prime
example of this. The loop was later fixed [2], and I can not help to
think that this fix caused people to look for further usages of the fork
construct.
Providing a better UX by increasing performance is good. Nevertheless,
this should not become a which hunt.
The usage of cargo_carte_uris/ngutes_uris/gradle_src_uri is not in a
large loop, therefore the situation is not comparable with the one in
the go-modules.eclass [2].
Which problem are we solving by moving away from this towards a slightly
more verbose construct?
- Flow
1: 59dbfb80f748 ("cargo.eclass: Add variable alternative to
$(cargo_crate_uris)")
https://github.com/gentoo/gentoo/commit/59dbfb80f748
2: 45e7aecd81e0 ("go-module.eclass: inline _go-module_gomod_encode()")
https://github.com/gentoo/gentoo/commit/45e7aecd81e0
OpenPGP_0x8CAC2A9678548E35.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
Offtopic: That calls for a YAS snippet ;D
W dniu 16.07.2023 o 15:40, Ulrich Mueller pisze:
On Sun, 16 Jul 2023, Maciej Barć wrote:
+case "${EAPI}" in
+ 7 | 8 )
+ :
+ ;;
+ * )
+ die "${ECLASS}: EAPI ${EAPI} unsupported."
+ ;;
+esac
The QA team has invested quite some work to unify that case block
between different eclasses. So, please stay with the standard style:
case ${EAPI} in
7|8) ;;
*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
esac
--
Have a great day!
~ Maciej XGQT Barć
x...@gentoo.org
Gentoo Linux developer
(dotnet, emacs, math, ml, nim, scheme, sci)
https://wiki.gentoo.org/wiki/User:Xgqt
9B0A 4C5D 02A3 B43C 9D6F D6B1 14D7 4A1F 43A6 AC3C
Re: [gentoo-dev] [PATCH 1/7] eclass/nuget.eclass: introduce new eclass
> On Sun, 16 Jul 2023, Maciej Barć wrote:
> +case "${EAPI}" in
> + 7 | 8 )
> + :
> + ;;
> + * )
> + die "${ECLASS}: EAPI ${EAPI} unsupported."
> + ;;
> +esac
The QA team has invested quite some work to unify that case block
between different eclasses. So, please stay with the standard style:
case ${EAPI} in
7|8) ;;
*) die "${ECLASS}: EAPI ${EAPI:-0} not supported" ;;
esac
signature.asc
Description: PGP signature
