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

[Refactor]: Cleanup CMake & separate libconky for all builds #2138

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Caellian
Copy link
Collaborator

@Caellian Caellian commented Jan 10, 2025

This PR does a lot and is a pain to review, I'm making it to "save progress". I'll highlight important changes once I'm done and it's no longer a draft.

Summary

  • Separate libconky from conky for all builds to allow lua modules to access conky internals.
  • Remove global variables that carried build sources/libs/dirs/options and replace them with targets.
  • Use targets for platform testing to ensure consistent results when building.
  • Replace build options stored in headers with compile definitions (stored in a target).
  • Remove some old files that weren't being used.

Rationale

Targets instead of variables

Mutating global state with include_directories is irreversible and makes it hard to debug/maintain the build files. It also leaks into subprojects and then all projects end up having access to everything which allows including headers (like "logging.h") which shouldn't be accessible from wrong places.
Having these stored in targets allows prepending include/library paths to patch things - like X11/X.h defining Always and None, which breaks tests if catch2 is included after some conky headers.

Removing config.h

Storing all these variables in -DFLAG=VALUE is convenient when targets are used, maintaining two separate lists of options (CMake and config.h.in) doesn't really provide any added benefit and is just annoying. There were already 2/3 options that were left behind and unused. Having the ConkyBuildOptions.cmake populate these automatically is super nice.

Also, as all options now use basically the same command:

conky_option(
  <option> "<help_text>" [<initial_value>]
  [DEPENDS <conditions>]
  [FALLBACK <value>]
  [SHOW_IF <conditions>]
  [TYPE (BOOL|FILE|DIR|STRING|INT|FLOAT)]
  [WARN "<warning>"]
)

it's possible to create a build_options.yaml that can be used for both docs and actual compile-time configuration.

Always using libconky

This comes up in #1899. Basically, cairo_imlib2_helper wants to access logging, but it can't because that would require bundling entire conky with it as #1899 adds .cc files (for global static LOGGER).
To deal with this, I think it's simplest to always separate libconky with PIC from main executable - this allows lua modules to access conky internals (like color parsing) without having to statically link to and include all conky sources. Alternatively, we could be binding a lot of the functions to Lua in order to make them accessible to modules, but that's a lot of work.

Work left to do

  • Fix lua module linking, currently static.
  • DEPENDENT_OPTION function doesn't seem to set correct fallback value when DEPENDS isn't met.

- Separate libconky from conky for all builds to allow lua modules to
  access conky internals.
- Remove global variables that carried build sources/libs/dirs/options
  and replace them with targets.
- Use targets for platform testing to ensure consistent results when
  building.
- Replace build options stored in headers with compile definitions
  (stored in a target).
- Remove some old files that weren't being used.

Signed-off-by: Tin ¿vagelj <[email protected]>
@Caellian Caellian marked this pull request as draft January 10, 2025 18:20
Copy link

netlify bot commented Jan 10, 2025

Deploy Preview for conkyweb canceled.

Name Link
🔨 Latest commit 211896e
🔍 Latest deploy log https://app.netlify.com/sites/conkyweb/deploys/678164dd0971b80008a7880d

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.

1 participant