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

Libretro core Windows fix #27

Merged
merged 6 commits into from
Oct 31, 2024
Merged

Libretro core Windows fix #27

merged 6 commits into from
Oct 31, 2024

Conversation

raidenii
Copy link

Fixes #25

@vadosnaprimer
Copy link

If you rebase on upstream devel branch, CI will upload an artifact for this PR, makes it easier to test for others.

@raidenii
Copy link
Author

Is this the corresponding CI build?
https://github.com/TASEmulators/freej2me-plus/actions/runs/11574364755

I don't think it builds libretro cores though, so not sure how it would help here.

@vadosnaprimer
Copy link

vadosnaprimer commented Oct 29, 2024

Isn't freej2me-lr.jar the file you need? It uploaded it fine after you pulled.

https://github.com/TASEmulators/freej2me-plus/actions/runs/11579312895/artifacts/2119080902

@raidenii
Copy link
Author

No, not really - my commit only deals with freej2me_libretro.c, and the workflow does not build that. I tested it locally myself though on both win32 and linux.

@AShiningRay
Copy link
Collaborator

Isn't freej2me-lr.jar the file you need? It uploaded it fine after you pulled.

https://github.com/TASEmulators/freej2me-plus/actions/runs/11579312895/artifacts/2119080902

Java CI won't get those libretro core changes at all, since nothing was changed in the java side, only the C core... i guess we could implement a CI for libretro later, but it's not a priority right now (especially since i don't have a windows machine at the moment so actual testing there will be lacking, as can be seen by this PR).

Now for the PR itself: Amazing work! Seems A-OK on my linux machine as well.

Copy link
Collaborator

@AShiningRay AShiningRay 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 to me, also made things more readable (at least to me).

Copy link
Collaborator

@AShiningRay AShiningRay left a comment

Choose a reason for hiding this comment

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

Fine by me as well, no complaints.

@AShiningRay
Copy link
Collaborator

Well the rest is just cosmetic so i don't see a need to add reviews to them, but they're also fine. Merging since i've found no issues with it. If anything arises on linux (unlikely), i can handle them over here. Thanks a bunch!

@AShiningRay AShiningRay merged commit 74ce9b7 into TASEmulators:devel Oct 31, 2024
1 check passed
AShiningRay pushed a commit that referenced this pull request Nov 19, 2024
This fixes relative path and saves folder on windows as well as improving readability in general for both it and linux. Spacing changes were also made to keep things consistent.
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.

Commit 9bd3628ca broke libretro core on Windows
3 participants