Skip to content

Remove the separate Pipewire profile type#3449

Closed
correctmost wants to merge 1 commit intoarchlinux:masterfrom
correctmost:cm/remove-pipewire-profile
Closed

Remove the separate Pipewire profile type#3449
correctmost wants to merge 1 commit intoarchlinux:masterfrom
correctmost:cm/remove-pipewire-profile

Conversation

@correctmost
Copy link
Copy Markdown
Contributor

PR Description:

This breaks an import cycle between applications/pipewire.py and models/audio_configuration.py:

  archinstall/default_profiles/applications/pipewire.py: error: Cycle detected in import chain
    archinstall/default_profiles/applications/pipewire.py
    archinstall/default_profiles/profile.py
    archinstall/lib/installer.py
    archinstall/lib/args.py
    archinstall/lib/models/audio_configuration.py (reportImportCycles)

This breaks an import cycle between applications/pipewire.py and
models/audio_configuration.py.
@correctmost correctmost requested a review from Torxed as a code owner May 8, 2025 19:57
@Torxed
Copy link
Copy Markdown
Member

Torxed commented May 9, 2025

I'm not sure if there was an actual import loop here?
And I personally think it will be easier to maintain if we keep the profiles split into profiles, and not within individual functions.

@correctmost
Copy link
Copy Markdown
Contributor Author

I'm not sure if there was an actual import loop here? And I personally think it will be easier to maintain if we keep the profiles split into profiles, and not within individual functions.

The import cycle doesn't cause issues at runtime because the PipewireProfile import is nested inside of a function:

def install_audio_config(
self,
installation: 'Installer'
) -> None:
info(f'Installing audio server: {self.audio.name}')
from ...default_profiles.applications.pipewire import PipewireProfile

Although the cycle doesn't cause runtime issues, it does suggest that there are design flaws.

The Profile interface has methods like is_graphic_driver_supported() and is_greeter_supported(), which don't seem relevant to Pipewire. I wonder if introducing a new class to handle application installs would be a good compromise between overusing Profile and adding more methods to Installer?

@CelestifyX
Copy link
Copy Markdown

oh no

@CelestifyX
Copy link
Copy Markdown

I totally agree with @Torxed, I think everything should have been in profiles, not in methods

@Torxed
Copy link
Copy Markdown
Member

Torxed commented May 9, 2025

Although the cycle doesn't cause runtime issues, it does suggest that there are design flaws.

The Profile interface has methods like is_graphic_driver_supported() and is_greeter_supported(), which don't seem relevant to Pipewire. I wonder if introducing a new class to handle application installs would be a good compromise between overusing Profile and adding more methods to Installer?

I agree that the design principles here are not ideal. And I do like your suggested approach of splitting up the Profile class. Perhaps something similar to:

graph TD;
    Profile-->Desktop;
    Profile-->Application;
Loading

They share a lot of overlap, but I agree that is_graphic_driver_supported doesn't make sense in a PipeWire profile :)

@svartkanin
Copy link
Copy Markdown
Collaborator

svartkanin commented May 10, 2025

I like that last suggestion @Torxed , I don't think having it in profiles makes sense as it's fundamentally something else, but having a new category application makes more sense.

This could be a different directory alongside profiles with many an interface class and exposed def install(installer) that each application needs to override. This is similar to the current implementation but with a new class Application rather as the base.

This means that the install class doesn't grow more and more but the installation steps sit with the applications.

@correctmost
Copy link
Copy Markdown
Contributor Author

I like the idea of having a new class for application installs.

I will close this PR for now. We can revisit the import cycle after the application code is cleaned up a bit.

@correctmost correctmost deleted the cm/remove-pipewire-profile branch May 16, 2025 21:18
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.

4 participants