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

Fix nm-applet requirement in meson.build #12714

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

timp87
Copy link

@timp87 timp87 commented Feb 4, 2025

The logic was swapped in 16cf990 which is incorrect.
So require nm-applet when have_networkmanager is true, i. e. disable_networkmanager is false. Not vice versa.

The logic was swapped in 16cf990 which is incorrect.
So require nm-applet when have_networkmanager is true, i. e. disable_networkmanager is false.
Not vice versa.
@timp87
Copy link
Author

timp87 commented Feb 4, 2025

@JosephMcc and @clefebvre please take a look.

@mtwebster
Copy link
Member

mtwebster commented Feb 4, 2025

The naming here just needs to be improved, but the logic is correct.

This section controls whether nm-applet is required by the session. Now that cinnamon has an internal implementation of the authentication dialog we do not want nm-applet to be a required part of the session.

@mtwebster mtwebster closed this Feb 4, 2025
mtwebster added a commit that referenced this pull request Feb 4, 2025
This does not change the default build but is more descriptive
about what the option is doing.

ref: #12714
@Fantu
Copy link
Contributor

Fantu commented Feb 5, 2025

@mtwebster also with new commit seems to remain the regression to don't have the possibility to disable network manager (for example for build that don't want it or archs that don't have it).
I mean about this (if build_nm_agent require libnm and if not require nm-applet but no more option to not have network manager):

if build_nm_agent
    session_conf.set('REQUIRED', '')
else
    session_conf.set('REQUIRED', 'nm-applet;')

@timp87
Copy link
Author

timp87 commented Feb 5, 2025

Thanks @Fantu
Exactly.
This was the reason why I thought the logic here is incorrect. I want to disable network manager, so why would it instead require nm-applet in such case?
I'm currently porting cinnamon to non-standard platform. So there are even more places where I have to delete nm artefacts in the build.

@mtwebster
Copy link
Member

mtwebster commented Feb 5, 2025

Our point was we don't want nm-applet required in the session when using our own agent. We didn't consider 'no network manager' when repurposing this option. I guess it'd be best to make another meson option to control this?

edit: or a multiple-choice option.

@mtwebster mtwebster reopened this Feb 5, 2025
@Fantu
Copy link
Contributor

Fantu commented Feb 5, 2025

One parameter with multiple-choice (like with_networkmanager=disabled|internal-agent|external-agent, or something better) or 2 parameters, for example return to have also disable_networkmanager and if true will disable also build_nm_agent.
Both option seems ok, do what you prefer.

@timp87
Copy link
Author

timp87 commented Feb 6, 2025

I would be happy if there was a way to fully disable network manager.
I can show more places where this full disablement should take place.

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.

3 participants