-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add custom cmake target to generate protocol headers during make installheaders
#6181
Conversation
Tested by only configuring, then running |
This will ensure the correct headers are generated before trying to install them.
make installheaders
make installheaders
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code PoV, LGTM. Builds fine manually and with Nix.
I'm waiting for you guys to confirm that hyprpm works as it should.
I tried to test it by compiling and installing to /usr/local/, and then restarting Hyprland, but when I run
so it can't check out the commit for some reason, and then it proceeds to configure Hyprland from the main branch. When it gets to the installheaders step, it seems to install the headers fine:
But when it then checks the installed headers, it reports a headers version mismatch:
So from what I can tell the problem isn't in my code, but in the fact that:
Is there a way to test hyprpm with a commit built from a different branch than main? From my manual testing (configuring (not building) hyprland and then running make installheaders), everything works fine. |
@zjeffer git confuses your |
Same issue, it's trying to find the branch I created in hyprpm/Hyprland, not in zjeffer/Hyprland:
Looks like hyprpm doesn't take into account forks at all. |
Yeah, it's really hard to test hyprpm when you are running code from an upstream pr/fork etc. I think it might work if you are running a branch of the main repo I wonder if we can come up with a way to have it run against PRs; then it can be CI step in the PR checks so it'll catch most potential hyprpm breakage before it hits main |
@fufexan Could you push my branch to the main repo, so I can test this out? |
Done, see the |
@fufexan Thanks, sadly it still doesn't work
Hyprpm seems untestable if the commit is not part of the main branch... I did test it manually by configuring Hyprland and then executing |
@zjeffer what distro are you on? The branch seems not to have been populated when you built Hyprland, and that's why it doesn't work. |
I'm on arch. I'll try out what's in yawor's comment soon |
I tried yawor's steps (but used This seems to be a different issue to what's in #6232 though. There the repo is not clonable at all. |
I think it's because I built from a detached head state, causing the branch to be empty. Here it shallow clones if the branch is main or empty: https://github.com/hyprwm/Hyprland/blob/main/hyprpm/src/core/PluginManager.cpp#L445. I'll build Hyprland from the actual branch now. |
It works! Here's the full output:
|
Great then, merging. |
Describe your PR, what does it fix/add?
As discussed in #6115, I think this is how we should do the protocol header generation: add a custom target that simply depends on the
OUTPUT
s of theadd_custom_command
s, and then execute that target in the installheaders step in the Makefile.This way, the headers will only be generated if necessary, and if they are already there, they won't be overridden.
I reapplied the reverted commit and added my changes in the second commit.
Is there anything you want to mention? (unchecked code, possible bugs, found problems, breaking compatibility, etc.)
Is it ready for merging, or does it need work?
ready to merge
CC @fufexan @cnt0