PCSX2 / pcsx2

PCSX2 - The Playstation 2 Emulator
https://pcsx2.net
GNU General Public License v3.0
11.89k stars 1.63k forks source link

[BUG]: update xz module submodule #5068

Closed aktau closed 3 years ago

aktau commented 3 years ago

Describe the Bug

NOTE: I discovered this due to the way libretro updates git repositories (https://github.com/libretro/pcsx2), but the same thing is present in https://github.com/pcsx2/pcsx2. I know that what the libretro-fetch.sh script does is not great: it forcibly sync every submodule to the latest commit on their master branch: git submodule foreach git pull origin master). But since it actually failed for PCSX2/xz, it got me looking.

The 3rdparty/xz/xz submodule is fixed at commit https://github.com/PCSX2/xz/commit/3d566cd519017eee1a400e7961ff14058dfaf33c due to https://github.com/PCSX2/pcsx2/commit/a26afbe9a1721ad0257074e591df526c68f36643.

Commit 3d566cd519017eee1a400e7961ff14058dfaf33c is only present on the v5.2 branch of that repo, so when pulling from master, merge conflicts occur.

Given that there have been a decent number of commits on master since then, perhaps it makes sense to update the submodule pointer? I don't know whether it's on purpose that this older version is retained, buy my gut says it isn't.

Reproduction Steps

$ git clone https://github.com/PCSX2/pcsx2.git
$ cd pcsx2
$ git submodule update --init --recursive
...
Submodule '3rdparty/xz/xz' (https://github.com/PCSX2/xz.git) registered for path '3rdparty/xz/xz'
...
remote: Enumerating objects: 8185, done.
remote: Counting objects: 100% (8185/8185), done.
remote: Compressing objects: 100% (1864/1864), done.
remote: Total 7853 (delta 6196), reused 7512 (delta 5947), pack-reused 0
Receiving objects: 100% (7853/7853), 1.40 MiB | 3.34 MiB/s, done.
Resolving deltas: 100% (6196/6196), completed with 255 local objects.
From https://github.com/PCSX2/xz
 * branch            3d566cd519017eee1a400e7961ff14058dfaf33c -> FETCH_HEAD
Submodule path '3rdparty/xz/xz': checked out '3d566cd519017eee1a400e7961ff14058dfaf33c'

$ git submodule foreach git pull origin master
git submodule foreach git pull origin master
Entering '3rdparty/xz/xz'
From https://github.com/PCSX2/xz
 * branch            master     -> FETCH_HEAD
Auto-merging windows/vs2013/config.h
Auto-merging windows/INSTALL-MinGW.txt
Auto-merging src/xz/xz.1
CONFLICT (content): Merge conflict in src/xz/xz.1
Auto-merging src/xz/file_io.h
Auto-merging src/xz/file_io.c
Auto-merging src/liblzma/lzma/lzma_decoder.c
Auto-merging src/liblzma/common/stream_decoder.c
CONFLICT (content): Merge conflict in src/liblzma/common/stream_decoder.c
Auto-merging src/liblzma/common/index_decoder.c
Auto-merging src/liblzma/common/index.c
Auto-merging src/liblzma/common/auto_decoder.c
CONFLICT (content): Merge conflict in src/liblzma/common/auto_decoder.c
Auto-merging src/liblzma/common/alone_decoder.c
CONFLICT (content): Merge conflict in src/liblzma/common/alone_decoder.c
Auto-merging src/liblzma/common/Makefile.inc
Auto-merging src/liblzma/api/lzma/version.h
CONFLICT (content): Merge conflict in src/liblzma/api/lzma/version.h
Auto-merging src/liblzma/api/lzma.h
Auto-merging src/liblzma/Makefile.am
CONFLICT (content): Merge conflict in src/liblzma/Makefile.am
Auto-merging dos/config.h
Auto-merging configure.ac
CONFLICT (content): Merge conflict in configure.ac
Auto-merging THANKS
Auto-merging NEWS
CONFLICT (content): Merge conflict in NEWS
Auto-merging INSTALL
CONFLICT (content): Merge conflict in INSTALL
Automatic merge failed; fix conflicts and then commit the result.
fatal: run_command returned non-zero status for 3rdparty/xz/xz
...

Expected Behavior

I expected being able to update to master at least without merge conflicts.

PCSX2 Revision

dev-18e0685ed4f191796c8e923caf4f5e96a930057e

Operating System

Linux (64bit) - Specify Distro Below

If Linux - Specify Distro

Debian

CPU

N/A

GPU

N/A

GS Settings

No response

Emulation Settings

No response

GS Window Screenshots

No response

Logs & Dumps

No response

stenzek commented 3 years ago

Is there any compelling reason to update the library version? (e.g. security, performance, etc).

Blindly updating dependencies is not a good idea, especially across major versions, as internal behaviour can change, causing regressions, APIs breaking compiling, etc.

refractionpcsx2 commented 3 years ago

It looks like all your submodules failed to update for some reason, it's not just xz. I suspect the problem is on your end :)

aktau commented 3 years ago

It looks like all your submodules failed to update for some reason, it's not just xz. I suspect the problem is on your end :)

Where do you see this? It only says Entering '3rdparty/xz/xz'.

Is there any compelling reason to update the library version? (e.g. security, performance, etc).

Blindly updating dependencies is not a good idea, especially across major versions, as internal behaviour can change, causing regressions, APIs breaking compiling, etc.

I agree. I just assumed it may be desirable because the referenced repo is under the PCSX2 organisation. On closer look it's just a mirror. The last commits are from 2018, which is two years after the current gitmodule commit but two years earlier than the latest in upstream (https://git.tukaani.org/?p=xz.git). I guess I assumed someone was doing manual things.

refractionpcsx2 commented 3 years ago

CONFLICT (content): Merge conflict in src/liblzma/common/stream_decoder.c CONFLICT (content): Merge conflict in src/liblzma/common/auto_decoder.c CONFLICT (content): Merge conflict in src/liblzma/common/alone_decoder.c CONFLICT (content): Merge conflict in src/liblzma/api/lzma/version.h CONFLICT (content): Merge conflict in src/liblzma/Makefile.am CONFLICT (content): Merge conflict in configure.ac CONFLICT (content): Merge conflict in NEWS CONFLICT (content): Merge conflict in INSTALL Automatic merge failed; fix conflicts and then commit the result.

All of this from your log has nothing to do with xz.

aktau commented 3 years ago

All of this from your log has nothing to do with xz.

I'm really not sure how you determine this. If we look at the repo, all of the mentioned files are present in PCSX2/xz. E.g.: https://github.com/PCSX2/xz/blob/master/src/liblzma/common/auto_decoder.c.

I've also inspected the merge conflicts manually, and they are within 3rdparty/xz/xz.

Lastly, I'd point out that while we see Entering '3rdparty/xz/xz', the same can be seen (but I had omitted it) for the other submodules, yet they print no such errors.

refractionpcsx2 commented 3 years ago

oh yeah, sorry, my bad.

orbea commented 3 years ago

git submodule foreach git pull origin master

Don't do that, to be blunt this is user error.

To clone submodules for the first time.

git submodule update --init --recursive

When updating them this seems to work robustly even in cases when submodules vanish or otherwise move.

git clean -xdff
git submodule foreach --recursive git clean -xdff
git submodule foreach --recursive git reset --hard
git submodule update --init --recursive
aktau commented 3 years ago

Don't do that, to be blunt this is user error.

I know that this (which the libretro-fetch.sh script does) is not great and should be fixed. I just thought not updating the dependency since 2016 may have been an oversight.

When updating them this seems to work robustly even in cases when submodules vanish or otherwise move.

Thanks (all), perhaps I'll contribute that fix to libretro.

orbea commented 3 years ago

I am no longer involved with libretro, but I noticed this by chance the other day.

https://github.com/libretro/libretro-super/issues/1583#issuecomment-976373840 https://github.com/libretro/libretro-super/issues/1584

aktau commented 3 years ago

Huh, that's unfortunate. It's a super convenient way of getting the latest retroarch + cores built with machine-specific optimizations and the newest compilers. I hope there's a replacement, guess I'll wait.