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

Fix osx build #43

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Conversation

Snowworm3000
Copy link

This is my first proper PR btw.

These are just some minor tweaks to the build commands to fix building on osx. The "cli-build" task just didn't run in the first place. There was an error when running the "copyzip" task, and the quick fix for me was to remove the chmod flag. There could be unintended side effects that I haven't noticed yet, so please let me know if this is an issue!

Thanks

Minor tweaks to the build commands to fix building on osx
@TrainDoctor TrainDoctor self-requested a review July 31, 2024 20:52
@TrainDoctor TrainDoctor added the enhancement New feature or request label Jul 31, 2024
Copy link
Member

@TrainDoctor TrainDoctor left a comment

Choose a reason for hiding this comment

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

Looks good, I'd recommend removing the windows entry for rsync since rsync is not a readily available command on windows.

@Snowworm3000
Copy link
Author

Thank you for reviewing my code!
I've just removed the Windows entry.

@Snowworm3000
Copy link
Author

Btw, I've also noticed another issue which is in the main.py file.

One of the comments says this:

# The decky plugin module is located at decky-loader/plugin
# For easy intellisense checkout the decky-loader code one directory up
# or add the `decky-loader/plugin` path to `python.analysis.extraPaths` in `.vscode/settings.json`

But the actual path is this:

decky-loader (repo) -> backend/decky_loader/plugin

Should I create a new PR to make this change, since it's sort of unrelated?

@TrainDoctor
Copy link
Member

Feel free to add it to this one. Better just to get it all up to date than to quibble.

@Snowworm3000
Copy link
Author

I've just realised the reason for this mismatch could be due to the decky-loader api 3.0 update.
So maybe these comments should only be changed once the decky-loader api update is released to the stable channel.
I'll try and test to see if this edit still works on the latest stable release, before reverting. But at least I can fix intellisense.

@Snowworm3000
Copy link
Author

I just checked the logs, and it appears to be working with these changes even on the latest stable release of decky-loader. The logging is still working when importing decky rather than decky_plugin. I presume this is ready to merge now.

@TrainDoctor
Copy link
Member

I just checked the logs, and it appears to be working with these changes even on the latest stable release of decky-loader. The logging is still working when importing decky rather than decky_plugin. I presume this is ready to merge now.

I'll give it a final look today when I have time. I appreciate you being attentive and looking to catch important details. So often these text changes are just used to inflate a user's contribution stats etc so genuine effort like this is always appreciated.

@Snowworm3000
Copy link
Author

Thank you! I'm developing my own plugin anyway, so I thought I'd push any changes I had to make to the template myself. This is one of the best repositories to work on as a beginner contributor.

Copy link
Member

@TrainDoctor TrainDoctor left a comment

Choose a reason for hiding this comment

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

Approved, will merge when Loader Pre-Release goes to stable.

@AAGaming00 AAGaming00 changed the base branch from v2 to main October 4, 2024 17:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: 👀 In review
Development

Successfully merging this pull request may close these issues.

2 participants