Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

opus: revert to autotools #23727

Merged
merged 1 commit into from
Mar 25, 2024
Merged

opus: revert to autotools #23727

merged 1 commit into from
Mar 25, 2024

Conversation

krant
Copy link
Contributor

@krant krant commented Mar 23, 2024

Maintainer: @thess @antonlacon
Compile tested: bcm53xx

Description:
Latest update in 6c3db5d has switched build system to Meson, which is broken on several non-SIMD platforms. Turns out, Meson support is not yet stable enough in the upstream, so we revert to autotools and drop meson-related patch.

Example of build error this PR fixes:

[135/168] Linking target src/libopus.so.0.10.0
FAILED: src/libopus.so.0.10.0 
arm-openwrt-linux-muslgnueabi-gcc  -o src/libopus.so.0.10.0 src/libopus.so.0.10.0.p/opus.c.o src/libopus.so.0.10.0.p/opus_decoder.c.o src/libopus.so.0.10.0.p/opus_encoder.c.o src/libopus.so.0.10.0.p/extensions.c.o src/libopus.so.0.10.0.p/opus_multistream.c.o src/libopus.so.0.10.0.p/opus_multistream_encoder.c.o src/libopus.so.0.10.0.p/opus_multistream_decoder.c.o src/libopus.so.0.10.0.p/repacketizer.c.o src/libopus.so.0.10.0.p/opus_projection_encoder.c.o src/libopus.so.0.10.0.p/opus_projection_decoder.c.o src/libopus.so.0.10.0.p/mapping_matrix.c.o src/libopus.so.0.10.0.p/analysis.c.o src/libopus.so.0.10.0.p/mlp.c.o src/libopus.so.0.10.0.p/mlp_data.c.o -L/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/usr/lib -L/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/lib -Wl,--as-needed -Wl,--no-undefined -fuse-ld=bfd -shared -fPIC -Wl,--start-group -Wl,-soname,libopus.so.0 -Wl,--whole-archive celt/libopus-celt.a silk/libopus-silk.a -Wl,--no-whole-archive -fuse-ld=bfd -znow -zrelro -lm -Wl,--end-group
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: src/libopus.so.0.10.0.p/opus_decoder.c.o: in function `opus_decoder_init':
opus_decoder.c:(.text+0x11c8): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: src/libopus.so.0.10.0.p/opus_decoder.c.o: in function `opus_dred_decoder_init':
opus_decoder.c:(.text+0x1ee0): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: src/libopus.so.0.10.0.p/opus_encoder.c.o: in function `opus_encoder_init':
opus_encoder.c:(.text+0x2174): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: src/libopus.so.0.10.0.p/opus_multistream_encoder.c.o: in function `opus_multistream_encoder_init_impl':
opus_multistream_encoder.c:(.text+0x224): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: src/libopus.so.0.10.0.p/analysis.c.o: in function `tonality_analysis_init':
analysis.c:(.text+0x236c): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: celt/libopus-celt.a.p/celt_decoder.c.o:celt_decoder.c:(.text+0x2db8): more undefined references to `opus_select_arch' follow
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: celt/libopus-celt.a.p/pitch.c.o: in function `pitch_search':
pitch.c:(.text+0xd60): undefined reference to `CELT_PITCH_XCORR_IMPL'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: celt/libopus-celt.a.p/celt_lpc.c.o: in function `_celt_autocorr':
celt_lpc.c:(.text+0xea0): undefined reference to `CELT_PITCH_XCORR_IMPL'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: silk/libopus-silk.a.p/init_decoder.c.o: in function `silk_reset_decoder':
init_decoder.c:(.text+0x24): undefined reference to `opus_select_arch'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: silk/libopus-silk.a.p/fixed_burg_modified_FIX.c.o: in function `silk_burg_modified_c':
burg_modified_FIX.c:(.text+0xc64): undefined reference to `CELT_PITCH_XCORR_IMPL'
/builder/shared-workdir/build/sdk/staging_dir/toolchain-arm_cortex-a9_gcc-12.3.0_musl_eabi/bin/../lib/gcc/arm-openwrt-linux-muslgnueabi/12.3.0/../../../../arm-openwrt-linux-muslgnueabi/bin/ld.bfd: silk/libopus-silk.a.p/fixed_pitch_analysis_core_FIX.c.o: in function `silk_pitch_analysis_core':
pitch_analysis_core_FIX.c:(.text+0xfc0): undefined reference to `CELT_PITCH_XCORR_IMPL'
collect2: error: ld returned 1 exit status

Latest update in 6c3db5d has switched build system to Meson,
which is broken on several non-SIMD platforms. Turns out,
Meson support is not yet stable enough in the upstream,
so we revert to autotools and drop meson-related patch.

Signed-off-by: krant <[email protected]>
@BKPepe
Copy link
Member

BKPepe commented Mar 23, 2024

Turns out, Meson support is not yet stable enough in the upstream

Is there any reference for it? Did you report it to upstream? If not, please do that.

@krant
Copy link
Contributor Author

krant commented Mar 23, 2024

Turns out, Meson support is not yet stable enough in the upstream

Is there any reference for it? Did you report it to upstream? If not, please do that.

xiph/opus#322 (comment)

@BKPepe
Copy link
Member

BKPepe commented Mar 23, 2024

In any case, your report is different hence report it to upstream to get this fixed.

@krant
Copy link
Contributor Author

krant commented Mar 23, 2024

In any case, your report is difference hence report it to upstream to get this fixed.

I have no idea what are you talking about. This PR is fixing build errors in OpenWrt project. If you want to talk about something else - I'm not interested.

@BKPepe
Copy link
Member

BKPepe commented Mar 24, 2024

Should I close this PR? I am thinking about it, so.

  • Your reference leads to the comment that it does not say anything that it is unstable or that it should not be used.
  • Opus developer said: improvements are definitely welcome
    Have you reported your issue to upstream? You did not, so why should we care about it? You refused to create an issue there, and you are not interested in it. You don't leave me any choice but to close this PR and revert your commit because it was not properly tested.

Sounds like you chose a random issue, and found something, that suits your case, well it isn't as your issue is:

opus_decoder.c:(.text+0x11c8): undefined reference to `opus_select_arch'

but the issue you linked is about AVX2. Once again, how relevant is it? It is not, so do you really think that we will narrow things upstream on your behalf? No way that won't happen.

@BKPepe BKPepe closed this Mar 24, 2024
@krant
Copy link
Contributor Author

krant commented Mar 24, 2024

@1715173329 @stangri @mhei @feckert @neheb @commodo @pprindeville @Ansuel
Dear maintaners of OpenWrt packages repo, I'm appealing to you regarding toxic and incompetent behavior of @BKPepe. This PR is just the latest example of many cases of his hostility towards contributors here. I'm urging you to take measures against it not only because it bothers me personally, but firstly because it harms OpenWrt community as a whole. How many contributors have turned away from OpenWrt because of this? How many will do? Why is a single person allowed to undermine the efforts of so many?

@1715173329 1715173329 reopened this Mar 24, 2024
@BKPepe
Copy link
Member

BKPepe commented Mar 24, 2024

There are some best principles which you should follow in the first place. I am glad that we do have this conversation.

Let's go back to the history:

  1. Your pull request - opus: update to 1.5.1 #23672
    What was the reason that your changes were included in one huge commit? I don't see it.
    Commit A - updating smth to recent version
    Commit B - changing build system

If something happened with the commit B, we could easily revert it.

  1. I see that opus developers are quite active and they are trying to help and fix issues. Why could you not report it to them to look into it? Your approach is offensive to me and you are just trying to let others merge some patches without being properly formatted, and not being included in upstream, which results in having them for ages in this repository results of a maintenance burden like it happened in python-yaml: fix build with Cython 3 #23481.

  2. What was the reason to switch it to meson? I could not see it in the commit description.

  3. Your commit should include in the commit description Fixes tag.

I was wondering if I should reply here something, but how time-consuming it was to write your comment instead of using your efforts more effectively and creating an opus developers issue about the issue that you experienced instead of reverting it?

I will leave you here some pages, which you should read and follow:
https://openwrt.org/submitting-patches
https://github.com/openwrt/packages/blob/master/CONTRIBUTING.md

@ynezz
Copy link
Member

ynezz commented Mar 24, 2024

BTW the package is clearly broken on buildbots as well, so IMO @1715173329 (who have reviewed and merged the initial contribution), have now one of the following options:

  1. fix the breakage together with @krant
  2. accept this pull request (which is basically revert of 6c3db5d) as it is

and handle the package update and switch to meson again properly, its not end of the world, right? We've Git reverts for exactly such reasons. Moreover we should consider adding such currently failing platform to the CI testing matrix, so we catch the build breakage here and not later.

@commodo
Copy link
Contributor

commodo commented Mar 24, 2024

So, things got heated here.
If people are having fun with this, I don't have any objections :)
I try not to judge what people consider to be fun.

To me, it looks like both parties read the message (from the other party) with a different tone that was probably intended.
Probably, some mistakes/misinterpretations were done, and some arguing came out of this (probably).
(These sort of arguments have been happening since the first text chats were invented; some people even went ahead and continued the argument in the streets; no idea how many beers were had after that, though ¯\_(ツ)_/¯ )

Keeping things at a technical level, I would also (just) vote on fixing the build error.
The way (about fixing this), is preference; whether we revert meson -> autotools, or kick upstream to fix it, doesn't make much difference (if it works). Reverting is faster now, fixing meson takes a bit of waiting on upstream; in the end either solution will have 1 or 2 commits.

I guess that's all I can add.

@1715173329
Copy link
Member

#23661 (comment)
However, krant is not the first person that rose this problem, and mostly, will not be the last one if we choose to ignore it.
Some developers choose to reduce their PR-s to minimize conflicts with him, and some just simply stop contributing to this repo (first-time contributors, mostly).

I don't know what happened to BKPepe, he seems to become very "irritable" since last year.
I'm not going to write a long post saying he is bad or any of that crap. But I'm really hoping, he can give more patience to our developers/contributors, especially our community really lacks active developers. Just see how long a PR will have to wait to be accepted, not only here, but also core repo.
People are not prefect, people are making mistakes, isn't it normal? I believe krant, and most other contributors here, will not intentionally try to break something.
I know it can be uncomfortable to review code, some times they may have some formatting errors, sometimes they may have just changed your code, but this is not the reason we shout to them.
So, please, give our contributors more patience.

Meson is not enforced, is not something that we have to use. We (try to) migrate to meson for faster compilation, just like CMake, it's simpler , it's the "modern" buildsystem.
And autotools in opus is verified (for years) to work stable, as a fix/solution for now, it's totally acceptable.
We can report this to upstream, we can improve meson build, but it needs time. Rollbacking to verified autotools is obviously the fastest way to make opus work again.

@neheb
Copy link
Contributor

neheb commented Mar 24, 2024

Yes, meson allows problem free and fast compilation. I’d like to see meson get fixed personally, but no problem reverting to auto tools for now. As upstream has said, auto tools is the primary build system. The others are not supported in the same way.

@commodo
Copy link
Contributor

commodo commented Mar 25, 2024

#23661 (comment)

I don't see @BKPepe in that discussion/thread.
I also don't follow closely discussions, unless I'm asked :p

But it now looks to me like an age-old "young-people clashing with old-people" issue.
Young people want to contribute & revolutionize a project; and old people like it just the way it is :)

FWIW: I haven't seen any good way to resolve this sort of conflict.
I do understand it, because I was once young(er).
I do know that my generation hasn't been allowed to express things as loudly (as in these days).
Expressing things loudly, isn't a bad thing, it's just different than in my time (when I had ~20 yrs).

Whether @BKPepe has become more irritable or not: ¯\_(ツ)_/¯
I am missing quite a bit of context; and I will admit, I will not make an effort to find out (due to lack of energy/time).

During my early contributor days, I've always tried to make things easier for the maintainer(s).
Though, I often failed (in my first contributions), because I got some things wrong.

Going back to the technical part, let's fix this.
Ideally the maintainers would give their word on this, and then we'd settle this more directly.
But I'm seeing that the maintainer system is getting a bit slow.

Copy link
Member

@ynezz ynezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@1715173329 1715173329 merged commit e616f87 into openwrt:master Mar 25, 2024
23 checks passed
@krant krant deleted the opus branch April 7, 2024 13:00
@BKPepe
Copy link
Member

BKPepe commented Apr 12, 2024

It's really a shame that there was an enormous discussion about who is wrong, who wants to cooperate with upstream. However, from my observation, I can see that some developers would appreciate it if the downstream cooperated with the upstream and vice versa. My intention to close this issue was to force the user to report a bug with upstream, which I did on his behalf, because no one else could do that and I was the bad guy. I see that @krant created a few PRs recently here, but he did not look at the issue further even I sent him reminder. (xiph/opus#327)

It was not my intention to be rude and I hope I was not, but it is what it is, though. The issue could be fixed in the upstream.

One wise guy told me that I should no longer help users, if they dont want to help and let the show that things could be and should be done entirely different. We could merge things like others wants, if thats what majority wants. I'm glad that people who aren't really that active in this repository joined this discussion. I will focus on my things, now! :)

Based on statistics, yeah. New contributors could have entirely different options than current ones, that happens over the time and I really doubt that if anyone could be still have enormous energy and review atleast 2-3 PRs day here without getting into any conflicts as I have been told.

@commodo
Copy link
Contributor

commodo commented Apr 15, 2024

One wise guy told me that I should no longer help users, if they dont want to help and let the show that things could be and should be done entirely different. We could merge things like others wants, if thats what majority wants. I'm glad that people who aren't really that active in this repository joined this discussion. I will focus on my things, now! :)

make some kids :)
you will see something similar;
i'm not saying this to offend anyone, but there is clearly some friction between 2 individuals of different ages;
i see this difference better, when i try to relate to my kids;
and also with how things have changed since i was a kid :)
the bigger the age difference, the higher the friction;

i try to be more ¯\_(ツ)_/¯

Based on statistics, yeah. New contributors could have entirely different options than current ones, that happens over the time and I really doubt that if anyone could be still have enormous energy and review atleast 2-3 PRs day here without getting into any conflicts as I have been told.

i've had my share of conflicts too;
what i learned from them is to be more ¯\_(ツ)_/¯
actually, i named my (small one-man) company Shruggie

@ynezz
Copy link
Member

ynezz commented Apr 15, 2024

make some kids :)

Make love, not war 🕊️

My intention to close this issue was to force the user to report a bug with upstream

This pull request contains a fix for a build failure, authored by the same person who introduced the issue. In my opinion, there were only two options: either fix it yourself or accept the proposed fix.

To my knowledge, there aren't any specific rules or guidelines that require contributors to report issues upstream before accepting fixes for build problems, moreover when the contributor clearly demonstrated, that he actually cares and made upstream aware via a comment.

However, there is an unofficial/unwritten "upstream first" philosophy that many of us try to adhere to when possible. This typically involves resolving the issue upstream and then backporting the fix downstream.

If there's any uncertainty, one can always refer to the OpenWrt Project Rules, specifically rule 12: "Be nice to each other" which can be applied as a fallback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants