search

miracle-wm-org / miracle-wm

Miracle is a Wayland tiling window manager built on Mir
https://miracle-wm.org
GNU General Public License v3.0
418 stars 14 forks source link

CMakeLists.txt: Drop PKG_CONFIG_PATH modification #211

Closed OPNA2608 closed 3 weeks ago

OPNA2608 commented 3 months ago

Fix https://github.com/mattkae/miracle-wm/pull/198#discussion_r1721453027

Setting up PKG_CONFIG_PATH with /usr/local/lib/pkgconfig, or ensuring that the correct Mir is on said path, shouldn't be this project's responsibility.

TibboddiT commented 3 months ago

@OPNA2608 If PKG_CONFIG_PATH end up not being set by the project, it makes sense to remove references to MIR_LIBRARIES_PKGCONFIG_DIRECTORY. In case you didn't see it, the initial issue involving creating this variable is #196.

mattkae commented 3 months ago

Hiya sorry for the delayed response, busy busy :)

I also had to think a little deeper on this. I also wonder if setting PKG_CONFIG_PATH is a good idea, given that it should be the override according to this comment. Maybe MIR_LIBRARIES_PKGCONFIG_DIRECTORY can be deleted altogether and we can just use PKG_CONFIG_PATH without setting it to begin with?

@TibboddiT would that work for you as well?

TibboddiT commented 2 months ago

@mattkae Yes, IMHO letting users set PKG_CONFIG_PATH is a standard thing. pkg_config will search the default path anyway:

A colon-separated (on Windows, semicolon-separated) list of directories to search for .pc files. The default directory will always be searched after searching the path; the default is libdir/pkgconfig:datadir/pkgconfig where libdir is the libdir for pkg-config and datadir is the datadir for pkg-config when it was installed.

On debian (and ubuntu I assume), pkg-config will search /usr/lib/pkgconfig, /usr/share/pkgconfig, /usr/local/lib/pkgconfig and /usr/local/share/pkgconfig after PKG_CONFIG_PATH.

I thought your specific local configuration and/or CI were the reasons why PKG_CONFIG_PATH was overridden, and I didn't want to distract you with build system issues since the project is still in WIP state.

If removing all references to PKG_CONFIG_PATH and MIR_LIBRARIES_PKGCONFIG_DIRECTORY is OK for you, the issue this PR address, and mine as well, will be solved.

@OPNA2608 Maybe you could update this PR, removing those variables, to check if CI is still green after that ?

Or should I open a new PR ?

mattkae commented 2 months ago

@mattkae Yes, IMHO letting users set PKG_CONFIG_PATH is a standard thing. pkg_config will search the default path anyway:

A colon-separated (on Windows, semicolon-separated) list of directories to search for .pc files. The default directory will always be searched after searching the path; the default is libdir/pkgconfig:datadir/pkgconfig where libdir is the libdir for pkg-config and datadir is the datadir for pkg-config when it was installed.

On debian (and ubuntu I assume), pkg-config will search /usr/lib/pkgconfig, /usr/share/pkgconfig, /usr/local/lib/pkgconfig and /usr/local/share/pkgconfig after PKG_CONFIG_PATH.

I thought your specific local configuration and/or CI were the reasons why PKG_CONFIG_PATH was overridden, and I didn't want to distract you with build system issues since the project is still in WIP state.

If removing all references to PKG_CONFIG_PATH and MIR_LIBRARIES_PKGCONFIG_DIRECTORY is OK for you, the issue this PR address, and mine as well, will be solved.

@OPNA2608 Maybe you could update this PR, removing those variables, to check if CI is still green after that ?

Or should I open a new PR ?

Sorry again for the late reply. Was away on holiday and my laptop broke :smile:

I think a new PR is worth it if @OPNA2608 agrees that it will solve her issues. The less environment variables to think about, the better in my opinion.

OPNA2608 commented 2 months ago

I can update this PR to try dropping all that setup.

TibboddiT commented 2 months ago

@OPNA2608 There is also a ref to MIR_LIBRARIES_PKGCONFIG_DIRECTORY at https://github.com/miracle-window-manager/miracle-wm/blob/develop/tests/CMakeLists.txt#L17. I think you can drop the if block completely.

OPNA2608 commented 2 months ago

Oops, you're right. Amended that.

TibboddiT commented 2 months ago

Tested locally, it's working for me. My build script feels more standard now :)

@mattkae I think this PR is ready to be merged.

mattkae commented 3 weeks ago

I dropped the ball on this one! I will look today :saluting_face: