| Message ID | 20221019220019.8418-2-moritz@h6t.eu |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Improve handling of debug builds in cargo packages | expand |
On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot <buildroot@buildroot.org> wrote: > > From: Moritz Bitsch <moritz@h6t.eu> > > The cargo --release flag is used to create optimized builds, not supplying > --release has major performance implications. Is there an env variable we can use to set this instead? Some packages use cargo without the cargo infrastructure, for example we have a number of pyo3(https://pyo3.rs/) based packages that use the cargo env but not the cargo build commands from here. > > Signed-off-by: Moritz Bitsch <moritz@h6t.eu> > --- > package/pkg-cargo.mk | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk > index f7e3f39503..e0556252fa 100644 > --- a/package/pkg-cargo.mk > +++ b/package/pkg-cargo.mk > @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS > $$($(2)_CARGO_ENV) \ > cargo build \ > --offline \ > - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ > + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ > --manifest-path Cargo.toml \ > --locked \ > $$($(2)_CARGO_BUILD_OPTS) > -- > 2.37.3 > > _______________________________________________ > buildroot mailing list > buildroot@buildroot.org > https://lists.buildroot.org/mailman/listinfo/buildroot
Am 20.10.22 um 00:12 schrieb James Hilliard: > On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot > <buildroot@buildroot.org> wrote: >> From: Moritz Bitsch <moritz@h6t.eu> >> >> The cargo --release flag is used to create optimized builds, not supplying >> --release has major performance implications. > Is there an env variable we can use to set this instead? > > Some packages use cargo without the cargo infrastructure, for example > we have a number of pyo3(https://pyo3.rs/) based packages that use the > cargo env but not the cargo build commands from here. I could not find any env variable for switching profiles. I had a look at python-rtoml, and the used setuptools-rust: supplying --debug to the "setup.py build" removes the --release flag from the cargo invocation. >> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> >> --- >> package/pkg-cargo.mk | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk >> index f7e3f39503..e0556252fa 100644 >> --- a/package/pkg-cargo.mk >> +++ b/package/pkg-cargo.mk >> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS >> $$($(2)_CARGO_ENV) \ >> cargo build \ >> --offline \ >> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ >> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ >> --manifest-path Cargo.toml \ >> --locked \ >> $$($(2)_CARGO_BUILD_OPTS) >> -- >> 2.37.3 >> >> _______________________________________________ >> buildroot mailing list >> buildroot@buildroot.org >> https://lists.buildroot.org/mailman/listinfo/buildroot
On Fri, Oct 21, 2022 at 2:48 AM Moritz Bitsch <moritz@h6t.eu> wrote: > > Am 20.10.22 um 00:12 schrieb James Hilliard: > > > On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot > > <buildroot@buildroot.org> wrote: > >> From: Moritz Bitsch <moritz@h6t.eu> > >> > >> The cargo --release flag is used to create optimized builds, not supplying > >> --release has major performance implications. > > Is there an env variable we can use to set this instead? > > > > Some packages use cargo without the cargo infrastructure, for example > > we have a number of pyo3(https://pyo3.rs/) based packages that use the > > cargo env but not the cargo build commands from here. > I could not find any env variable for switching profiles. > > I had a look at python-rtoml, and the used setuptools-rust: supplying > --debug to > the "setup.py build" removes the --release flag from the cargo invocation. Hmm, there are two pyo3 build backends, only setuptools-rust uses setup.py, we also have maturin which is pep517 based. > >> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> > >> --- > >> package/pkg-cargo.mk | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk > >> index f7e3f39503..e0556252fa 100644 > >> --- a/package/pkg-cargo.mk > >> +++ b/package/pkg-cargo.mk > >> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS > >> $$($(2)_CARGO_ENV) \ > >> cargo build \ > >> --offline \ > >> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ > >> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ > >> --manifest-path Cargo.toml \ > >> --locked \ > >> $$($(2)_CARGO_BUILD_OPTS) > >> -- > >> 2.37.3 > >> > >> _______________________________________________ > >> buildroot mailing list > >> buildroot@buildroot.org > >> https://lists.buildroot.org/mailman/listinfo/buildroot
Am 21.10.22 um 18:41 schrieb James Hilliard: > On Fri, Oct 21, 2022 at 2:48 AM Moritz Bitsch <moritz@h6t.eu> wrote: >> >> Am 20.10.22 um 00:12 schrieb James Hilliard: >> >>> On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot >>> <buildroot@buildroot.org> wrote: >>>> From: Moritz Bitsch <moritz@h6t.eu> >>>> >>>> The cargo --release flag is used to create optimized builds, not supplying >>>> --release has major performance implications. >>> Is there an env variable we can use to set this instead? >>> >>> Some packages use cargo without the cargo infrastructure, for example >>> we have a number of pyo3(https://pyo3.rs/) based packages that use the >>> cargo env but not the cargo build commands from here. >> I could not find any env variable for switching profiles. >> >> I had a look at python-rtoml, and the used setuptools-rust: supplying >> --debug to >> the "setup.py build" removes the --release flag from the cargo invocation. > > Hmm, there are two pyo3 build backends, only setuptools-rust uses setup.py, > we also have maturin which is pep517 based. I'm in no way a a python expert, so take my words with a grain of salt. I could not find a way to specify what build profile is used by maturin when in a pep517 build. Even if maturin supports the -r flag for release builds when called directly. > >>>> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> >>>> --- >>>> package/pkg-cargo.mk | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk >>>> index f7e3f39503..e0556252fa 100644 >>>> --- a/package/pkg-cargo.mk >>>> +++ b/package/pkg-cargo.mk >>>> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS >>>> $$($(2)_CARGO_ENV) \ >>>> cargo build \ >>>> --offline \ >>>> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ >>>> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ >>>> --manifest-path Cargo.toml \ >>>> --locked \ >>>> $$($(2)_CARGO_BUILD_OPTS) >>>> -- >>>> 2.37.3 >>>> >>>> _______________________________________________ >>>> buildroot mailing list >>>> buildroot@buildroot.org >>>> https://lists.buildroot.org/mailman/listinfo/buildroot
On Fri, Oct 21, 2022 at 5:04 PM Moritz Bitsch <moritz@h6t.eu> wrote: > > Am 21.10.22 um 18:41 schrieb James Hilliard: > > On Fri, Oct 21, 2022 at 2:48 AM Moritz Bitsch <moritz@h6t.eu> wrote: > >> > >> Am 20.10.22 um 00:12 schrieb James Hilliard: > >> > >>> On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot > >>> <buildroot@buildroot.org> wrote: > >>>> From: Moritz Bitsch <moritz@h6t.eu> > >>>> > >>>> The cargo --release flag is used to create optimized builds, not supplying > >>>> --release has major performance implications. Looking at this further the cargo --release flag merely tells cargo to use the release profile rather than the dev profile, however the issue of optimizations being enabled or not is really a matter of what the profile configuration is, right now we are effectively choosing between the default release or default dev profile configuration which does not properly match the build settings. Instead what we really want to do is configure all cargo profiles based on the selected buildroot build settings. Unfortunately cargo doesn't have any env variable that selects a profile, but we can sidestep this simply by configuring both root profiles(which other profiles inherit from). I think the existing cargo build flags are fine, I sent a patch which should fix the profile configurations so that they match the buildroot build settings, let me know if this fixes the optimization issue for you: https://lore.kernel.org/buildroot/20221021224448.3502942-1-james.hilliard1@gmail.com/ > >>> Is there an env variable we can use to set this instead? > >>> > >>> Some packages use cargo without the cargo infrastructure, for example > >>> we have a number of pyo3(https://pyo3.rs/) based packages that use the > >>> cargo env but not the cargo build commands from here. > >> I could not find any env variable for switching profiles. > >> > >> I had a look at python-rtoml, and the used setuptools-rust: supplying > >> --debug to > >> the "setup.py build" removes the --release flag from the cargo invocation. > > > > Hmm, there are two pyo3 build backends, only setuptools-rust uses setup.py, > > we also have maturin which is pep517 based. > > I'm in no way a a python expert, so take my words with a grain of salt. > > I could not find a way to specify what build profile is used by maturin > when in a pep517 build. Even if maturin supports the -r flag for release > builds when called directly. I think the real issue is actually not that the wrong build profile is being used so much as the build profile configuration itself is wrong. > > > > >>>> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> > >>>> --- > >>>> package/pkg-cargo.mk | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk > >>>> index f7e3f39503..e0556252fa 100644 > >>>> --- a/package/pkg-cargo.mk > >>>> +++ b/package/pkg-cargo.mk > >>>> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS > >>>> $$($(2)_CARGO_ENV) \ > >>>> cargo build \ > >>>> --offline \ > >>>> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ > >>>> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ > >>>> --manifest-path Cargo.toml \ > >>>> --locked \ > >>>> $$($(2)_CARGO_BUILD_OPTS) > >>>> -- > >>>> 2.37.3 > >>>> > >>>> _______________________________________________ > >>>> buildroot mailing list > >>>> buildroot@buildroot.org > >>>> https://lists.buildroot.org/mailman/listinfo/buildroot
Am 22.10.22 um 00:57 schrieb James Hilliard: > On Fri, Oct 21, 2022 at 5:04 PM Moritz Bitsch <moritz@h6t.eu> wrote: >> >> Am 21.10.22 um 18:41 schrieb James Hilliard: >>> On Fri, Oct 21, 2022 at 2:48 AM Moritz Bitsch <moritz@h6t.eu> wrote: >>>> >>>> Am 20.10.22 um 00:12 schrieb James Hilliard: >>>> >>>>> On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot >>>>> <buildroot@buildroot.org> wrote: >>>>>> From: Moritz Bitsch <moritz@h6t.eu> >>>>>> >>>>>> The cargo --release flag is used to create optimized builds, not supplying >>>>>> --release has major performance implications. > > Looking at this further the cargo --release flag merely tells cargo to use the > release profile rather than the dev profile, however the issue of optimizations > being enabled or not is really a matter of what the profile configuration is, > right now we are effectively choosing between the default release or default > dev profile configuration which does not properly match the build settings. > > Instead what we really want to do is configure all cargo profiles based on the > selected buildroot build settings. > > Unfortunately cargo doesn't have any env variable that selects a profile, but > we can sidestep this simply by configuring both root profiles(which other > profiles inherit from). > > I think the existing cargo build flags are fine, I sent a patch which should fix > the profile configurations so that they match the buildroot build settings, let > me know if this fixes the optimization issue for you: > https://lore.kernel.org/buildroot/20221021224448.3502942-1-james.hilliard1@gmail.com/ Thanks, for the detailed response. The mentioned patch works fine for my use case. I can reproduce the performance issue/improvement when switching between different optimization levels. >>>>> Is there an env variable we can use to set this instead? >>>>> >>>>> Some packages use cargo without the cargo infrastructure, for example >>>>> we have a number of pyo3(https://pyo3.rs/) based packages that use the >>>>> cargo env but not the cargo build commands from here. >>>> I could not find any env variable for switching profiles. >>>> >>>> I had a look at python-rtoml, and the used setuptools-rust: supplying >>>> --debug to >>>> the "setup.py build" removes the --release flag from the cargo invocation. >>> >>> Hmm, there are two pyo3 build backends, only setuptools-rust uses setup.py, >>> we also have maturin which is pep517 based. >> >> I'm in no way a a python expert, so take my words with a grain of salt. >> >> I could not find a way to specify what build profile is used by maturin >> when in a pep517 build. Even if maturin supports the -r flag for release >> builds when called directly. > > I think the real issue is actually not that the wrong build profile is > being used > so much as the build profile configuration itself is wrong. After looking into some crates with different behavior when built in debug or release mode (for example rust-embed) I think that most of them check for enabled debug_assertions, which you handle in your patch. I think the conditional --release switch should be removed. Your patch is by far the more precise solution. >> >>> >>>>>> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> >>>>>> --- >>>>>> package/pkg-cargo.mk | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk >>>>>> index f7e3f39503..e0556252fa 100644 >>>>>> --- a/package/pkg-cargo.mk >>>>>> +++ b/package/pkg-cargo.mk >>>>>> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS >>>>>> $$($(2)_CARGO_ENV) \ >>>>>> cargo build \ >>>>>> --offline \ >>>>>> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ >>>>>> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ >>>>>> --manifest-path Cargo.toml \ >>>>>> --locked \ >>>>>> $$($(2)_CARGO_BUILD_OPTS) >>>>>> -- >>>>>> 2.37.3 >>>>>> >>>>>> _______________________________________________ >>>>>> buildroot mailing list >>>>>> buildroot@buildroot.org >>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
On Mon, Oct 24, 2022 at 5:15 AM Moritz Bitsch <moritz@h6t.eu> wrote: > > Am 22.10.22 um 00:57 schrieb James Hilliard: > > On Fri, Oct 21, 2022 at 5:04 PM Moritz Bitsch <moritz@h6t.eu> wrote: > >> > >> Am 21.10.22 um 18:41 schrieb James Hilliard: > >>> On Fri, Oct 21, 2022 at 2:48 AM Moritz Bitsch <moritz@h6t.eu> wrote: > >>>> > >>>> Am 20.10.22 um 00:12 schrieb James Hilliard: > >>>> > >>>>> On Wed, Oct 19, 2022 at 6:02 PM Moritz Bitsch via buildroot > >>>>> <buildroot@buildroot.org> wrote: > >>>>>> From: Moritz Bitsch <moritz@h6t.eu> > >>>>>> > >>>>>> The cargo --release flag is used to create optimized builds, not supplying > >>>>>> --release has major performance implications. > > > > Looking at this further the cargo --release flag merely tells cargo to use the > > release profile rather than the dev profile, however the issue of optimizations > > being enabled or not is really a matter of what the profile configuration is, > > right now we are effectively choosing between the default release or default > > dev profile configuration which does not properly match the build settings. > > > > Instead what we really want to do is configure all cargo profiles based on the > > selected buildroot build settings. > > > > Unfortunately cargo doesn't have any env variable that selects a profile, but > > we can sidestep this simply by configuring both root profiles(which other > > profiles inherit from). > > > > I think the existing cargo build flags are fine, I sent a patch which should fix > > the profile configurations so that they match the buildroot build settings, let > > me know if this fixes the optimization issue for you: > > https://lore.kernel.org/buildroot/20221021224448.3502942-1-james.hilliard1@gmail.com/ > > Thanks, for the detailed response. The mentioned patch works fine for my > use case. I can reproduce the performance issue/improvement when > switching between different optimization levels. Great, can you reply to that patch with a "Tested-by:" so that patchwork picks it up? > > >>>>> Is there an env variable we can use to set this instead? > >>>>> > >>>>> Some packages use cargo without the cargo infrastructure, for example > >>>>> we have a number of pyo3(https://pyo3.rs/) based packages that use the > >>>>> cargo env but not the cargo build commands from here. > >>>> I could not find any env variable for switching profiles. > >>>> > >>>> I had a look at python-rtoml, and the used setuptools-rust: supplying > >>>> --debug to > >>>> the "setup.py build" removes the --release flag from the cargo invocation. > >>> > >>> Hmm, there are two pyo3 build backends, only setuptools-rust uses setup.py, > >>> we also have maturin which is pep517 based. > >> > >> I'm in no way a a python expert, so take my words with a grain of salt. > >> > >> I could not find a way to specify what build profile is used by maturin > >> when in a pep517 build. Even if maturin supports the -r flag for release > >> builds when called directly. > > > > I think the real issue is actually not that the wrong build profile is > > being used > > so much as the build profile configuration itself is wrong. > > After looking into some crates with different behavior when built in > debug or release mode (for example rust-embed) I think that most of them > check for enabled debug_assertions, which you handle in your patch. There are still ways for cargo packages to potentially change behavior based on the selected profile even when the profile configurations are otherwise identical. > > I think the conditional --release switch should be removed. Your patch > is by far the more precise solution. I think we should probably still leave the --release switch as it's possible for cargo package build scripts to detect the configured profile using the PROFILE build script env variable which gets set by cargo: https://doc.rust-lang.org/cargo/reference/environment-variables.html#environment-variables-cargo-sets-for-build-scripts There's probably not many cases where this will matter but it does appear to be more correct to set the --release switch as well when possible. > > >> > >>> > >>>>>> Signed-off-by: Moritz Bitsch <moritz@h6t.eu> > >>>>>> --- > >>>>>> package/pkg-cargo.mk | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk > >>>>>> index f7e3f39503..e0556252fa 100644 > >>>>>> --- a/package/pkg-cargo.mk > >>>>>> +++ b/package/pkg-cargo.mk > >>>>>> @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS > >>>>>> $$($(2)_CARGO_ENV) \ > >>>>>> cargo build \ > >>>>>> --offline \ > >>>>>> - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ > >>>>>> + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ > >>>>>> --manifest-path Cargo.toml \ > >>>>>> --locked \ > >>>>>> $$($(2)_CARGO_BUILD_OPTS) > >>>>>> -- > >>>>>> 2.37.3 > >>>>>> > >>>>>> _______________________________________________ > >>>>>> buildroot mailing list > >>>>>> buildroot@buildroot.org > >>>>>> https://lists.buildroot.org/mailman/listinfo/buildroot
diff --git a/package/pkg-cargo.mk b/package/pkg-cargo.mk index f7e3f39503..e0556252fa 100644 --- a/package/pkg-cargo.mk +++ b/package/pkg-cargo.mk @@ -119,7 +119,7 @@ define $(2)_BUILD_CMDS $$($(2)_CARGO_ENV) \ cargo build \ --offline \ - $$(if $$(BR2_ENABLE_DEBUG),,--release) \ + $$(if $$(BR2_ENABLE_RUNTIME_DEBUG),,--release) \ --manifest-path Cargo.toml \ --locked \ $$($(2)_CARGO_BUILD_OPTS)