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

Plugin API: Add version query #3545

Merged
merged 3 commits into from
Oct 14, 2023
Merged

Plugin API: Add version query #3545

merged 3 commits into from
Oct 14, 2023

Conversation

vaxerski
Copy link
Member

I know hyprctl version exists but this is better.

cc @fufexan @outfoxxed @Duckonaut RFC

@vaxerski vaxerski requested a review from fufexan October 10, 2023 19:30
fufexan
fufexan previously approved these changes Oct 10, 2023
@Duckonaut
Copy link
Contributor

Very very good to have. I won't need to parse shell command output querying the state of the program I'm using, was a really roundabout way to do it. Info seems well organized, happy that ignoring the dirty tag is easier here as well. I'll make hyprload rely on this as soon as a versioned release with this exists, don't want to break compatibility for non-git users

@vaxerski
Copy link
Member Author

I'll make hyprload rely on this as soon as a versioned release with this exists, don't want to break compatibility for non-git users

I'd still gatekeep it under some #ifdef for people on < 0.31

@outfoxxed
Copy link
Contributor

Looks good to me. It might also be a good idea to have a standard check in place you can opt out of that would show a hyprland notification along the lines of "plugin {} was compiled for a different hyprland version and could not be loaded." (implemented in the header so its compiled on the plugin side)

@vaxerski
Copy link
Member Author

Plugins have to do that themselves, I'll gatekeep all the official plugins with that once this is merged and versioned like duckonaut.

@outfoxxed
Copy link
Contributor

Do you have any reason not to implement a standard check for it in the header?

@vaxerski
Copy link
Member Author

if you have an idea on how to do that, feel free

@outfoxxed
Copy link
Contributor

You implement the check in the header so it gets compiled plugin side, and uses the GIT_COMMIT_HASH of the headers its compiling against. I can implement it later if you want.

@vaxerski
Copy link
Member Author

how do I make it not compile hl-side though?

@outfoxxed
Copy link
Contributor

have a plugin side api header, in that header define a function to get the commit it was compiled against. it dosen't matter if its compiled hyprland side because you're only checking the result from the function in the plugin.

@Duckonaut
Copy link
Contributor

I'd still gatekeep it under some #ifdef for people on < 0.31

Yeah, I could do that as well, i'll just need to tweak the build system to handle that.

@vaxerski
Copy link
Member Author

fair point, I'll see tomorrow if I can do it

@vaxerski
Copy link
Member Author

maybe not very tomorrow but have a look guys :)

@Duckonaut
Copy link
Contributor

For maximum compatibility, maybe it could be useful to return this as a C str just using .c_str() for a stable ABI? Since it will be called across plugin boundaries. Not strictly necessary since i mean, most of hyprland doesn't care for stable ABI and returns std::strings in lots of places. Either way is cool tho, good to have

@vaxerski
Copy link
Member Author

good point.

@outfoxxed
Copy link
Contributor

I think it would be useful to have hyprland compare the result of __hyprland_api_get_hash by default (maybe some kind of opt out) and show an error via the notification system that the plugin couldn't be loaded if it mismatches.

@vaxerski
Copy link
Member Author

that can be done in a follow-up

@outfoxxed
Copy link
Contributor

yeah otherwise LGTM

@vaxerski
Copy link
Member Author

although I am opposed to that

@vaxerski vaxerski requested a review from fufexan October 13, 2023 19:40
@outfoxxed
Copy link
Contributor

although I am opposed to that

I feel like an opt-out makes sense because most plugins have to touch a lot more than the "HyprlandAPI" header surface to do anything useful. What's your reasoning?

@vaxerski
Copy link
Member Author

many don't break between rebuilds, like e.g. csgo-vulkan-fix that had been working at one point for 4 months without an issue for me.

it'd be better to make it a warning rather than error

@outfoxxed
Copy link
Contributor

Yeah I'm not opposed to that, I just want there to be a visual indication something is/may be wrong by default, because a ton of people get ABI breakage all the time with hy3.

@vaxerski
Copy link
Member Author

why don't they just use hyprload :)

@outfoxxed
Copy link
Contributor

its been broken with hy3 for quite a while (since the pkg config change). I think echoduck fixed it like a couple days ago but I haven't checked yet.

@Duckonaut
Copy link
Contributor

Yeah it wasn't properly serving its own headers ever since the pkgconfig change, in the meantime I think literally every plugin moved to pkg-config so now that I made it serve headers via that, it works well again. It still always worked for downloading and updating plugins, just didn't actually help with building them correctly since May

@vaxerski
Copy link
Member Author

since fuffie is dead I will just merge it

@vaxerski vaxerski merged commit d5a572b into main Oct 14, 2023
20 checks passed
@memchr
Copy link
Contributor

memchr commented Oct 14, 2023

Meson breaks down magnificently.
Did you forget to include version.h in src/plugins/PluginAPI.hpp?

@vaxerski
Copy link
Member Author

all checks passed tho? wot

@memchr
Copy link
Contributor

memchr commented Oct 14, 2023

../src/plugins/PluginAPI.hpp: In function ‘const char* __hyprland_api_get_hash()’:
../src/plugins/PluginAPI.hpp:276:12: error: ‘GIT_COMMIT_HASH’ was not declared in this scope
  276 |     return GIT_COMMIT_HASH;
      |            ^~~~~~~~~~~~~~~
../src/plugins/PluginAPI.hpp:276:12: note: the macro ‘GIT_COMMIT_HASH’ had not yet been defined
In file included from ../src/plugins/../helpers/../macros.hpp:17,
                 from ../src/plugins/../helpers/Vector2D.hpp:5,
                 from ../src/plugins/../includes.hpp:148,
                 from ../src/plugins/../defines.hpp:1,
                 from ../src/plugins/../Compositor.hpp:7,
                 from ../src/plugins/PluginAPI.cpp:2:
../src/plugins/../helpers/../version.h:2: note: it was later defined here
    2 | #define GIT_COMMIT_HASH    "21b5cf402afb8a72ffbf12efdd0a0d46849d86dd"
      |

From gcc, meson setup build then ninja -C build

@memchr
Copy link
Contributor

memchr commented Oct 14, 2023

This will also cause plugin build to fail, so I think it's safe to say that include version.h is needed.

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.

None yet

5 participants