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