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

sharedgpures: Rebase sharedgpures patches for e553be7 and move previous version to legacy #866

Merged
merged 1 commit into from
Nov 5, 2022

Conversation

Iglu47
Copy link
Contributor

@Iglu47 Iglu47 commented Sep 24, 2022

This commit based on work of @llde (The same as #861, but avoided monolithic style patches and splitting redesign. But, as I said before, because of such significant changes, it is likely that if any problems arise, it will first be necessary to additionally check that this is not due to the influence of a rebase.)

resolves #860

Apart from this, minor changes (that I have long wanted to do): slightly more respect patch separation, more cleanup for non-fshack version and fix some compilation warning (for legacy versions also where is it possible).

@Iglu47
Copy link
Contributor Author

Iglu47 commented Sep 24, 2022

this PR does not include updates related to wine-mirror/wine@7384591...f664af0

…us version to legacy

Based on Frogging-Family#861

Co-authored-by: llde <[email protected]>
Tested-by: Dmitry Skvortsov <[email protected]>
@Iglu47 Iglu47 marked this pull request as draft September 24, 2022 14:15
@Iglu47
Copy link
Contributor Author

Iglu47 commented Sep 24, 2022

I face into failing of patch applying on 7.14, this was broken before so let's leave it to another PR and not complicate this one for now (seems to require an additional version to be created due to one line =/ )

@Iglu47 Iglu47 marked this pull request as ready for review September 24, 2022 15:21
@llde
Copy link
Contributor

llde commented Sep 24, 2022

At first glance, it doesn't seems there is anything wrong. I checked the 2 problematic places where I was having issues in the fence patchset and it seems correct.
Do we want to necessarely handle every wine breakage, by providing a version even for older releases? It's honestly seem like a sinkhole, and unless a version have some programs working that were broken and never fixed, seems useless. Not sure what is Tkg policy in this case however.

I'm still not convinced that this splitting is the best one instead of a clear separation of components, for the reasons I said in my last comment at #861, and also I find this kind of successive commits that edit the same function that was edited or added by a previous commit confunding at best, while I'm able to follow easier a single commit that make all relevant changes (or at least multiple commits that make indipendent changes to a component). Does keeping the commit have a benefit? I think only if we can apply them in the repo as commits (as git am do), that currently this project doesn't do.
Regarding checking the influence of a rebase, can't we do this by simply checking out an older version of tkg and forcing an older buildable commit of wine mainline and staging? If a bug happens only with the patchset enabled (or part of), and it happens only with the rebase version of the patchset, but not in an older version, then we have an high chance it's an issue with the rebase (there is still the possibility that a wine change cause a regression in a game only when certain features of the patchset are detected, so it just seems the patchset is causing the issue). Seems hardly a point in favour of keeping a (imho) disfunctional patch separation in that way.

But again, I'm just a wine enthusiast from when I were in the middle school, around 12 years old, in the 2007-8. The final answer is for TkG to make, I just wanted to make clear my reasons in making the patchset that way.

@Tk-Glitch
Copy link
Member

@llde I quite liked your approach with #861 tbh but since Iglu47 has been, so far, maintaining this patch series here, I want them to have co-decision rights on this. If they feel like it'll make the maintenance burden worse for them, I can understand how they find it undesirable (it's not necessarily true for you and me, but we all have our own ways, right?).

@llde
Copy link
Contributor

llde commented Sep 28, 2022

@Tk-Glitch I do agree. I started the discussion just to understand if my reasoning was sound or not.

@Tk-Glitch
Copy link
Member

So, since I can't really wrap my head around this, I'll merge as-is and we'll go from there.

@Iglu47 Do you feel like @llde 's approach isn't desirable for you? Any input on this?

@Tk-Glitch Tk-Glitch merged commit 16fc2d9 into Frogging-Family:master Nov 5, 2022
@Iglu47
Copy link
Contributor Author

Iglu47 commented Nov 5, 2022

As I said at once: the monolithic style and adding the extra step of working with the patchset is not too big deal for me (because I'm not a developer - my time is not very valuable). The original #861 for 7.17 contained too many unnecessary changes compared to the patches from Proton. That's why I did some cleaning (although it's still not perfect).

@llde
Copy link
Contributor

llde commented Nov 5, 2022

Would it be possible to for now warn about the patchset being applyied into more recents version of wine? Currently isn't applicable in 7.20

@Iglu47 Iglu47 deleted the sharedgpures-7.17 branch November 23, 2022 18:01
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.

Shared GPU resources patchset doesn't apply
3 participants