Skip to content

build: add opt-in installation of QML lib #4

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

Merged
merged 1 commit into from
Aug 25, 2024

Conversation

nydragon
Copy link

Hey, this follows our conversation on Matrix regarding lsp and linting support, while maybe not a desired default feature, it might be interesting to expose an option for folks to enable if they do want to give that a shot.

Override the package with withQMLLib = true; to enable lib installation, alternatively add -DINSTALL_QML_LIB=ON to your cmake build command.

@nydragon
Copy link
Author

Please let me know if I should add an instruction to the README.md or BUILD.md (or anywhere else really).

Copy link
Member

@outfoxxed outfoxxed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume you tested this and it actually works, given that I've got a couple nits.
Also yeah you could put it in build.md.

@nydragon
Copy link
Author

Yep, has been tested:

qmllint without the QS lib

╰─>$ qmllint src/shell.qml -I /nix/store/1bwc1r6y30c7s32vi4dh9c3jwwlwmcjh-qtdeclarative-6.7.2/lib/qt-6/qml/
Warning: src/shell.qml:1:1: Warnings occurred while importing module "Quickshell": [import]
import Quickshell
^^^^^^
---
Warning: Failed to import Quickshell. Are your import paths set up properly? [import]
---

Warning: src/shell.qml:6:1: Scope was not found. Did you add all import paths? [import]

^^^^^
Warning: src/shell.qml:4:1: ShellRoot was not found. Did you add all import paths? [import]
ShellRoot {
^^^^^^^^^
Warning: src/shell.qml:5:5: Cannot assign to non-existent default property [missing-property]
    Bar {}
    ^^^

with the lib linked

┬─[ny@marr:~/.c/quickshell]─[22:26:21]─[G:main=]─[nix-shell]
╰─>$ qmllint src/shell.qml -I /nix/store/1bwc1r6y30c7s32vi4dh9c3jwwlwmcjh-qtdeclarative-6.7.2/lib/qt-6/qml/ -I /nix/store/drqfbjn8cz93b0mvadld05dgiiwywban-quickshell-0.1.0/lib/qt-6/qml/
┬─[ny@marr:~/.c/quickshell]─[22:27:58]─[G:main=]─[nix-shell]
╰─>$

Although I have some, presumably nix related, issues with qmllint finding the lib, but that also happened with the builtin QML features, so I think it is unrelated. Will implement the feedback

@nydragon nydragon force-pushed the master branch 2 times, most recently from 041b36f to 2ba8f11 Compare August 25, 2024 20:51
Override the package with `withQMLLib = true;` to enable lib
installation, alternatively add `-DINSTALL_QML_LIB=ON` to your cmake
build command.

Co-authored-by: a-usr <[email protected]>
@nydragon
Copy link
Author

nydragon commented Aug 25, 2024

I applied all requests

@outfoxxed
Copy link
Member

Alright, LGTM. Do keep in mind that qmllint/qmlls are not entirely accurate in the context of quickshell, as quickshell papers over some oddities like qmldir files even needing to exist.
Quickshell also does a couple things like injecting types into a module at runtime that will break it, which might be fixed later.

@outfoxxed outfoxxed merged commit b40d414 into quickshell-mirror:master Aug 25, 2024
@nydragon
Copy link
Author

Alright, noted! Thanks for the approval

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