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

add config option to choose between cheevos implementations #8501

Merged
merged 11 commits into from Apr 20, 2019

Conversation

Projects
None yet
8 participants
@meleu
Copy link
Contributor

commented Mar 20, 2019

Description

Adding the option cheevos_rcheevos_enable, if true RetroArch uses the new cheevos implementation, otherwise, use the old one. This option was created for debugging only and is intended to be temporary.

If everything works fine with the new implementation, later I'll submit another PR removing the old one.

Why is this "needed"?

Last time @leiradel tried to integrate rcheevos in RetroArch (#7321) it brought some issues on the Android build (as reported by @fr500 in #7341) and the PR had to be reverted. And as it's a pain to reproduce the same toolchain setup as the buildbot, the debugging was really tough.

My approach here is based on a suggestion from @fr500 so we can try to reproduce those issues while don't break anything.

I've been testing an Android binary built by @Jamiras on Windows Visual Studio (he had to do some tweaks to get it compiled on VS) and those issues reported by @fr500 don't happen. Here's the apk if someone is interested in testing: retroarch-activity-memref.apk).

This PR also updates rcheevos files.

Related Issues

#7341
RetroAchievements/rcheevos#3
RetroAchievements/rcheevos#7

Reviewers

@leiradel @fr500 @Jamiras

@fr500

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

wow... big PR.
Why 111 files?

@fr500

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

I'm pretty sure you need the griffin stuff.

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

This pull request introduces 4 alerts when merging cc63738 into c5d9a69 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same
  • 1 for Wrong type of arguments to formatting function

Comment posted by LGTM.com

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

@meleu Does the new cheevos implementation require --enable-lua?

@meleu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

Why 111 files?

88 files are from deps/rcheevos [EDIT: 81 were deps/rcheevos/test, which isn't required here, then I removed them.]
22 files are due the namespacing thing.
1 file is the griffin/griffin.c (which seems to be failing in the Travis CI build)

I'm pretty sure you need the griffin stuff.

alright, I'm going to put them back. They were removed as part of tweaks Jamiras did to compile the Android build on Visual Studio, but it seems to be unnecessary here.

This pull request introduces 4 alerts

Thanks, I'll work on that.

Does the new cheevos implementation require --enable-lua?

yes, but since rcheevos wasn't integrated in RetroArch yet, we don't have anything using Lua for now. Then it's not really required right now.

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

This pull request introduces 4 alerts when merging 016d5c3 into 7b9fcfd - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same
  • 1 for Wrong type of arguments to formatting function

Comment posted by LGTM.com

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

yes, but since rcheevos wasn't integrated in RetroArch yet, we don't have anything using Lua for now. Then it's not really required right now.

I'm not sure I understand what you mean by recheevos not being integrated? Isn't that why its in deps?

https://github.com/libretro/RetroArch/tree/master/deps/rcheevos

My concern is that lua which is hardly used is already causing several issues and it seems more pain than gain in my opinion.

For example RetroArch will crash inside the system lua when playing video content when both --enable-mpv and --enable-lua are used. We also seem to be missing some defines when building lua leading the warnings at least on linux during the linking stage, but they are all highly platform specific making it hard to fix... Of course there is the issue with rcheevos requiring lua5.3 which makes it pretty much impossible to support a system lua given to missing features in upstream lua (pkg-config file and shared library). See issue RetroAchievements/rcheevos#15 for more info on the third point.

I know this doesn't directly relate to your goal here, but I think its worth mentioning that this is a bit of a sore spot...

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

88 files are from deps/rcheevos (cause we already use rcheevos on RAchievements emulators and Jamiras has been working on more features for it in the last months)

81 files are in rcheevos/test, which isn't really required for RetroArch. I recommend excluding that directory for this PR.

I'm pretty sure you need the griffin stuff.

I had to introduce a new griffin file to prevent conflicts between the old cheevos and new cheevos structures (perhaps more things need to be namespaced or shared). I also had to include two new include paths (deps/rcheevos/include and deps/lua/src). I did this to one project file. Is there any easy way to do it to all necessary projects? Or at least some way to identify all affected projects?

since rcheevos wasn't integrated in RetroArch yet, we don't have anything using Lua for now. Then it's not really required right now.

rcheevos needs to know about the lua types/functions to compile, but we're not actually creating/passing around any lua objects yet. I haven't been part of the discussions around lua compatibility/versioning/etc, so I don't really have any insight to add about that.

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

I haven't been part of the discussions around lua compatibility/versioning/etc, so I don't really have any insight to add about that.

Basically it should be fixed in upstream lua and this would not be an issue, but given that every distro reinvents the wheel fixing it themselves I am not going to hold my breath...

@meleu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2019

I'm not sure I understand what you mean by recheevos not being integrated? Isn't that why its in deps?

I meant the rcheevos is not integrated in the official releases of RetroArch. Its files are there, but only for those who compile with --enable-new_cheevos (and I have a feeling that I'm the only person who actually does it).

Regarding Lua, there isn't any achievement coded with it (and our back-end is not yet ready for it).

To be fair, rcheevos not integrated in RetroArch is the bottleneck for the evolution of the RAchievements feature. We are holding many cool improvements (such as coding achievements with Lua) to avoid incompatibilities with RetroArch.

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

If we can switch between new and old achievements in the menu/configuration file then it should be fine to enable HAVE_NEW_CHEEVOS by default assuming it ready? The problems I have experienced seem limited to lua and as I don't use achievements I wasn't sure how useful the new cheevos was without lua.

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Mar 20, 2019

Seems this doesn't work for osx + travis, maybe just something missing from griffin?

@meleu meleu changed the title add config option to choose between cheevos implementations WIP: add config option to choose between cheevos implementations Mar 22, 2019

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 23, 2019

This pull request introduces 4 alerts when merging 415d168 into 4258836 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same
  • 1 for Wrong type of arguments to formatting function

Comment posted by LGTM.com

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

This pull request introduces 4 alerts when merging aeac5f8 into 4258836 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same
  • 1 for Wrong type of arguments to formatting function

Comment posted by LGTM.com

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

This pull request introduces 3 alerts when merging 38e01de into 4258836 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same

Comment posted by LGTM.com

@meleu meleu force-pushed the meleu:runtime_rcheevos_switch branch from 38e01de to c66b866 Mar 24, 2019

@bparker06

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

This pull request introduces 3 alerts when merging c66b866 into fcd55d8 - view on LGTM.com

new alerts:

  • 3 for Comparison result is always the same

Comment posted by LGTM.com

@meleu meleu changed the title WIP: add config option to choose between cheevos implementations add config option to choose between cheevos implementations Mar 24, 2019

@meleu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 24, 2019

Unless someone has something more to comment, this PR is now ready to be merged.

This pull request introduces 3 alerts when merging c66b866 into fcd55d8 - view on LGTM.com

The 3 alerts mentioned by LGTM are all related to a comparison like this:

unsigned long value;
/* ... */
if (value > 0xffffffffU)

And the alert is: Comparison is always false because value <= 4294967295.

However those checks were added as a way to prevent wrong 32-bit values on 64-bit platforms (as we can see in this commit from @leiradel)

@leiradel

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

These warnings are harmless, and they will happen on all platforms where sizeof(long) == sizeof(int) I believe, regardless of the LGTM analysis.

As for the PR, I think it's good to go, and an important step in phasing out the old cheevos code and putting RetroArch on par with the official Retro Achievements emulators.

meleu added a commit to RetroAchievements/rcheevos that referenced this pull request Mar 24, 2019

@meleu meleu referenced this pull request Mar 24, 2019

Merged

prevent "always false" alerts #18

@meleu

This comment has been minimized.

Copy link
Contributor Author

commented Mar 25, 2019

Here's an android build with all commits until now: https://www.dropbox.com/s/n0fvy5s27g57e8g/retroarch-activity-memref.apk?dl=1

@leiradel

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Ok, RetroArch was reading the configuration from ~/.config/retroarch/retroarch.cfg instead of the current folder. I can see the message saying that I'm using the new implementation, and I could get a couple of SMW cheevos. I've switched back to the old implementation and was again able to get the same cheevos.

It looks good to me. Should I look for something else in specific?

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2019

@meleu If you don't mind, can you please use git rebase instead of merging the master branch into your PR?

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

This will avoid this ugly merge commits. :)

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

rebasing makes collaboration hard. Anyone who has this branch checked out will end up with a detached head because rebasing rewrites history. If you're concerned about clean history, squash and merge when pulling the code into master.

@meleu meleu force-pushed the meleu:runtime_rcheevos_switch branch from 1c352ff to 9091409 Apr 20, 2019

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2019

rebasing makes collaboration hard

Not at all, you can use git fetch to get new changes and can use git rebase or git reset as needed to merge collaboration or reset the repo state.

If you're concerned about clean history, squash and merge when pulling the code into master.

In practice this never happens and now look at our git history....

@fr500

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

rebasing is essential to ensure your PR works on top of the current master.
the way I work usually is:

git remote add upstream $url_of_upstream_repo
git fetch upstream
git checkout $local_branch --------> most people just work on master
git rebase upstream/master -------> optionally use git rebase -i to squash commits that can be combined
--fix conflicts if any--
git push -f

You shouldn't end up with a detached head anywhere doing this

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Rebasing is great for local changes, and should be used judiciously for keeping any local changes out of your local master. But once you've pushed your branch, you've created a node that others might be dependent on. As soon as you rebase/forcepush after that, you destroy the upstream node and create a new one. If no one else is using that node then there aren't any problems. If someone is, then they have to jump through a lot of hoops to get back into a working state.

From git's official documentation:

Rebasing doesn't play well with pull requests, because you can't see what minor changes someone made if they rebased (incidentally, the consensus inside the Bitbucket Server development team is to never rebase during a pull request).

Rebasing can be dangerous! Rewriting history of shared branches is prone to team work breakage. This can be mitigated by doing the rebase/squash on a copy of the feature branch, but rebase carries the implication that competence and carefulness must be employed.

I've worked at two companies who have used git, and both have insisted that you should not rebase once you've pushed.

However, this discussion is irrelevant to the changes in this pull request. If libretro's official stance is to use rebase in PRs, then by all means we should do so.

@twinaphex twinaphex merged commit bca306a into libretro:master Apr 20, 2019

4 checks passed

LGTM analysis: C/C++ No new or fixed alerts
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
LGTM analysis: Python No code changes detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@orbea

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2019

@meleu You broke C89_BUILD=1 for cheevos...

CC cheevos/cheevos.c
CC cheevos/badges.c
CC cheevos/var.c
CC cheevos/cond.c
CC cheevos-new/cheevos.c
cheevos/var.c: In function ‘cheevos_var_get_memory’:
cheevos/var.c:298:4: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
    rarch_system_info_t* system = runloop_get_system_info();
    ^~~~~~~~~~~~~~~~~~~
CC cheevos-new/fixup.c
CC cheevos-new/parser.c
CC cheevos-new/hash.c
CC deps/rcheevos/src/rcheevos/trigger.c
In file included from cheevos-new/cheevos.c:67:
deps/rcheevos/include/rcheevos.h:161:4: error: ISO C90 doesn’t support unnamed structs/unions [-Werror=pedantic]
   };
    ^
CC deps/rcheevos/src/rcheevos/condset.c
cheevos-new/cheevos.c: In function ‘rcheevos_test_leaderboards’:
cheevos-new/cheevos.c:676:13: error: C++ style comments are not allowed in ISO C90
             // this is where we would update the onscreen tracker
             ^
cheevos-new/cheevos.c:676:13: error: (this will be reported only once per input file)
cc1: some warnings being treated as errors
In file included from cheevos-new/fixup.c:23:
deps/rcheevos/include/rcheevos.h:161:4: error: ISO C90 doesn’t support unnamed structs/unions [-Werror=pedantic]
   };
    ^
cc1: some warnings being treated as errors
make: *** [Makefile:201: obj-unix/release/cheevos-new/cheevos.o] Error 1
make: *** Waiting for unfinished jobs....
make: *** [Makefile:199: obj-unix/release/cheevos-new/fixup.o] Error 1
In file included from deps/rcheevos/src/rcheevos/internal.h:4,
                 from deps/rcheevos/src/rcheevos/trigger.c:1:
deps/rcheevos/include/rcheevos.h:161:4: error: ISO C90 doesn’t support unnamed structs/unions [-Werror=pedantic]
   };
    ^
cc1: some warnings being treated as errors
make: *** [Makefile:199: obj-unix/release/deps/rcheevos/src/rcheevos/trigger.o] Error 1
In file included from deps/rcheevos/src/rcheevos/internal.h:4,
                 from deps/rcheevos/src/rcheevos/condset.c:1:
deps/rcheevos/include/rcheevos.h:161:4: error: ISO C90 doesn’t support unnamed structs/unions [-Werror=pedantic]
   };
    ^
cc1: some warnings being treated as errors
make: *** [Makefile:199: obj-unix/release/deps/rcheevos/src/rcheevos/condset.o] Error 1
@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Seems like something that should have been caught by one of the CI checks.

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2019

I have a commit to add those to travis now although it will make CI even slower, but I'll wait for a fix first...

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

C89 fix here: meleu@1c0efbf
Do you want me to add it to this branch/PR, create a new PR to merge to this branch, or a new PR to merge directly to libretro/master?

@libretro libretro deleted a comment from orbea Apr 20, 2019

@meleu

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

I suppose a new PR to RetroArch/master is easier/simpler.

@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

It's also CXX_BUILD that has to be fixed -

also @orbea I cannot be blamed for merging this when I was continually nudged into merging this PR over and over again, and then it turns out these build checks were not being run. It's damned if you do, damned if you don't over here.

What we should do is add CXX_BUILD/C89_BUILD checks to Travis so that it gets reviewed for every PR automatically. If something isn't automated, you can expect people to overlook it. So let's solve this in a pragmatic way instead of assuming everybody is going to run tests for every single PR. Hint: they won't, and you can't reliably guarantee that, with some Travis / PR automation steps you can.

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

The CXX_BUILD failure is unrelated to this feature:

CC gfx/drivers_context/wayland_ctx.c
gfx/drivers_context/wayland_ctx.c:609:1: error: invalid conversion from ‘void (*)()’ to ‘void (*)(void*, wl_surface*, wl_output*)’ [-fpermissive]
 };
 ^
In file included from /usr/include/wayland-client-core.h:30:0,
                 from /usr/include/wayland-client.h:39,
                 from gfx/drivers_context/wayland_ctx.c:20:
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_toplevel_config(void*, xdg_toplevel*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:639:5: error: invalid conversion from ‘void*’ to ‘const uint32_t* {aka const unsigned int*}’ [-fpermissive]
     wl_array_for_each(state, states) {
     ^
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_zxdg_toplevel_config(void*, zxdg_toplevel_v6*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:708:5: error: invalid conversion from ‘void*’ to ‘const uint32_t* {aka const unsigned int*}’ [-fpermissive]
     wl_array_for_each(state, states) {
     ^
gfx/drivers_context/wayland_ctx.c: In function ‘void registry_handle_global(void*, wl_registry*, uint32_t, const char*, uint32_t)’:
gfx/drivers_context/wayland_ctx.c:871:33: error: invalid conversion from ‘void*’ to ‘output_info_t* {aka output_info*}’ [-fpermissive]
       output_info_t *oi = calloc(1, sizeof(output_info_t));
                           ~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~
@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

That's on Linux though. On Windows, those files don't get compiled so it arrives at the Cheevos files instead, and there are definitely errors there.

I'll fix those errors you pointed out and commit it so that you can see it for yourself.

@fr500

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

@Jamiras while I understand the logic behind not rebasing, fact is RetroArch is somewhat fragile.
So having commits neatly packed together make things easier to bisect.

@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Hi @Jamiras ,

7b3be5e

Can you tell me what CXX_BUILD errors remain for you after this commit?

@orbea

This comment has been minimized.

Copy link
Collaborator

commented Apr 20, 2019

I'm at this with the CXX_BUILD.

menu/cbs/menu_cbs_ok.c: In function ‘int generic_action_ok_displaylist_push(const char*, const char*, const char*, unsigned int, size_t, size_t, unsigned int)’:
menu/cbs/menu_cbs_ok.c:282:37: error: invalid conversion from ‘int’ to ‘msg_hash_enums’ [-fpermissive]
    menu_displaylist_info_t info = {0};
                                     ^
CC menu/cbs/menu_cbs_scan.c
make: *** [Makefile:201: obj-unix/release/menu/cbs/menu_cbs_ok.o] Error 1
make: *** Waiting for unfinished jobs....
@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

I've got at least three files still complaining:

menu/cbs/menu_cbs_ok.c: In function ‘int generic_action_ok_displaylist_push(const char*, const char*, const char*, unsigned int, size_t, size_t, unsigned int)’:
menu/cbs/menu_cbs_ok.c:282:37: error: invalid conversion from ‘int’ to ‘msg_hash_enums’ [-fpermissive]
    menu_displaylist_info_t info = {0};
                                     ^
In file included from /usr/include/wayland-client-core.h:30:0,
                 from /usr/include/wayland-client.h:39,
                 from gfx/drivers_context/wayland_ctx.c:20:
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_toplevel_config(void*, xdg_toplevel*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:666:4: error: lvalue required as left operand of assignment
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c:666:4: error: lvalue required as increment operand
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_zxdg_toplevel_config(void*, zxdg_toplevel_v6*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:745:4: error: lvalue required as left operand of assignment
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c:745:4: error: lvalue required as increment operand
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers/gl.c: In function ‘void gl2_load_texture_image(GLenum, GLint, GLint, GLsizei, GLsizei, GLint, GLenum, GLenum, const GLvoid*)’:
gfx/drivers/gl.c:359:31: error: invalid conversion from ‘unsigned int’ to ‘gl_capability_enum’ [-fpermissive]
    if (gl_check_capability(cap) && internalFormat != GL_BGRA_EXT)
                               ^
@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Pushed another buildfix -

499235c

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

I think this is the last one:

CC gfx/drivers_context/wayland_ctx.c
In file included from /usr/include/wayland-client-core.h:30:0,
                 from /usr/include/wayland-client.h:39,
                 from gfx/drivers_context/wayland_ctx.c:20:
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_toplevel_config(void*, xdg_toplevel*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:666:4: error: lvalue required as left operand of assignment
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c:666:4: error: lvalue required as increment operand
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c: In function ‘void handle_zxdg_toplevel_config(void*, zxdg_toplevel_v6*, int32_t, int32_t, wl_array*)’:
gfx/drivers_context/wayland_ctx.c:745:4: error: lvalue required as left operand of assignment
    wl_array_for_each((void*)state, states)
    ^
gfx/drivers_context/wayland_ctx.c:745:4: error: lvalue required as increment operand
    wl_array_for_each((void*)state, states)
    ^
Makefile:199: recipe for target 'obj-unix/release/gfx/drivers_context/wayland_ctx.o' failed
make: *** [obj-unix/release/gfx/drivers_context/wayland_ctx.o] Error 1
@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

@Jamiras OK, so it turned out wl_array_for_each is a macro inside a Wayland header file that is not compatible with CXX_BUILD - so I had to make a modified version of the macro for it to work -

8e638f4

@Jamiras

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

That seemed to clear it up. I'm seeing the errors in cheevos-new now. Will address and open a new PR.

@twinaphex

This comment has been minimized.

Copy link
Member

commented Apr 20, 2019

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.