-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Adding compilation support for Fedora 42 #6780
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the build still works (although, as of writing, the test suite is still running). Assuming that passes too, I'm fine to approve this with the one minor correction noted below.
It would be nice to have Fedora support (I plan to move over there at some point), but that being said, Fedora is not officially supported by Webots, and I think its unlikely that the maintainers are going to go through the work to change that. Thus, I'm not sure if its wise to add Fedora support as it's unlikely to be maintained when dependency requirements change.
A possible compromise might be to add an echo statement warning of that fact, and let Fedora become a quasi-supported OS. (We can also probably make a note of that somewhere on the wiki.) However, that still leaves a chunk of explicitly-unmaintained code in the repo.
@omichel can probably make the final call here.
UPDATE: Test suite passes! Only failing test is robot_window_html.wbt
(See #6755)
Co-authored-by: CoolSpy3 <55305038+CoolSpy3@users.noreply.github.com>
I agree with @CoolSpy3, we should add an echo statement warning that Fedora support is still experimental and not officially documented. Then, we could merge this PR. |
@calvarado2004 I'm trying to test this PR on Fedora 42. Did you manage to find a way to get around the include errors in wren? I'm currently stuck on the one in I'm thinking I'm going to have to set another preprocessor directive and include |
Okay; I've managed to get a working Fedora build! Keynotes:
As of now, CI is passing, and my build passes all but two of the tests on Fedora. (I'll investigate tomorrow; it's late now.) So, pending that, I'm good to merge this! (Will approve once that's done.) Keep in mind that I've been testing on Fedora 42. (I'll update the PR title to reflect that.) @calvarado2004 If you want to test against Fedora 41, feel free to do so and report your findings, but I will not be explicitly testing against it otherwise. Thank you for the work you put into getting this started! BONUS NOTE: I just realized I actually broke the Ubuntu 22.04 check in the test suite, but the test still passed, so it's possible that that code is no longer needed. I'll wait till tomorrow to remove it so that we get at least 3 CI runs with it (hopefully passing). [The comment says that it's from the 22.04 BETA, so it's possible it's been fixed in the latest release.] If it passes, we can probably just remove the check. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, here's what I found with the tests:
billboard.wbt
- Fails when run in the test suite but passes in a normal Webots session or when the suite is run through xvfb
. Based on the output, it looks like its failing due to a window-size bug. Given Fedora's unofficial support status and the fact that this is only a problem in the suite, I don't think it's worth fixing.
supervisor_start_stop_movie.wbt
- This was failing due to a missing video codec. Fedora's repos normally don't include proprietary codecs. I updated the install script to enable RPMFusion and the full (proprietary) version of ffmpeg
, and that appears to have fixed the problem.
As for the Ubuntu 22.04 test. It does appear to pass consistently (~3 consecutive times so far with no failures), so I think it's good to re-enable. However, even though it's a small change, I think it makes sense to address it in another PR, so I won't back it out here.
Regardless, I'm now happy with the state of this PR, so I'm going to approve!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you.
Description
Adding compilation support for Fedora 41
Documentation
If this pull-request changes the installation scripts for Linux, adding support for Fedora 41, among Ubuntu.