search

iXit / wine-nine-standalone

Build Gallium Nine support on top of an existing WINE installation
GNU Lesser General Public License v2.1
272 stars 23 forks source link

Misc cleanups #154

Closed 9ary closed 1 year ago

9ary commented 1 year ago

This fixes minor issues with the build system, in preparation for another pr to address #150.

9ary commented 1 year ago

Okay, it looks like the CI isn't gonna agree with me... Seems like ubuntu 22.04 forces us to go to wine 7.0. Maybe there's a way to get newer meson on 20.04 instead?

dhewg commented 1 year ago

Just skimmed over it for now, but some general comments:

https://github.com/dhewg/wine-nine-standalone/commits/22.04 BUT: https://github.com/iXit/wine-nine-standalone/pull/146#issuecomment-1453642497 So we actually want to stay on 20.04 for maximum release binary compatibility.

About the meson stuff: We just switched to 20.04 for the releases, so I think it's time to bump our minimal meson required here to whatever 20.04 ships and then fix warnings with that version. We should make sure any further changes to the build system still works with that minimal meson version, but the 20.04 CI setup already tests that. In doubt, we can check CI artifacts upon push.

On the WINE side, if you build from this repo locally yourself, building with ancient WINE versions still produces working binaries for that version. I'd prefer to keep that feature, unless there's a good reason not to. But in the worst case, any changes needs to work with at least WINE v6.0.

9ary commented 1 year ago

Yeah, currently what requires 0.54 is get_external_property, but get_cross_property (deprecated since 0.58) should work here. If we can conditionally use the newer method on a recent enough version then I would prefer that, otherwise I'm happy to just use the deprecated method.

9ary commented 1 year ago

So it looks like doing things depending on the Meson version is possible:

if meson.version().version_compare('>= 0.56')
  pe_dir = get_option('libdir') / 'wine' / meson.get_external_property('pe_dir')
  so_dir = get_option('libdir') / 'wine' / meson.get_external_property('so_dir')
else
  pe_dir = get_option('libdir') / 'wine' / meson.get_cross_property('pe_dir')
  so_dir = get_option('libdir') / 'wine' / meson.get_cross_property('so_dir')
endif

However, for simplicity, I decided to leave it out. It prints the following message which is benign (not a warning, so it's fine).

NOTICE: Future-deprecated features used:
 * 0.56.0: {'gui_app arg in executable', 'dependency.get_pkgconfig_variable'}
 * 0.58.0: {'meson.get_cross_property'}

Minimum supported version is now 0.49 since that's when the / path concatenation operator was introduced.

Now to address the next CI failure... Edit: well, crap. Wine 6.0's winegcc doesn't check for -Wb,--fake-module, so the .fake suffix is still required there. What's more worrying is that winebuild doesn't support --data-only which might foil my plans...

9ary commented 1 year ago

Oh for crying out loud, it looks like one of the deprecation fixes breaks things on that old version of meson.

dhewg commented 1 year ago

Changing the directory structure is problematic, since winetricks relies on it: https://github.com/Winetricks/winetricks/blob/20230212/src/winetricks#L10083 So I wouldn't change it for just cleanup reasons. It's just the tar release anyway, the instance installing it can do whatever is desired.

And I think it should be enough to add gallium-nine-standalone.tar.gz to .gitignore? That's used if running release.sh without -o. If one uses that switch, its most likely pointing to another dir.

9ary commented 1 year ago

Changing the directory structure is problematic, since winetricks relies on it: https://github.com/Winetricks/winetricks/blob/20230212/src/winetricks#L10083 So I wouldn't change it for just cleanup reasons. It's just the tar release anyway, the instance installing it can do whatever is desired.

Well, the commit message already justifies the change: for system installs, ninja install will now install things in the correct locations, which makes life easier for packagers. It also makes it possible to point WINEDLLPATH at the wine dir in the release tarball instead of running the install script.

Winetricks is basically just another distro, and it is expected that they would have to adapt when packaging the new release. This is fairly common so I don't really see a problem here. If you like, it's possible to make the tarball keep the old structure, but that would get messy, and I think correctness is worth the minor inconvenience.

And I think it should be enough to add gallium-nine-standalone.tar.gz to .gitignore? That's used if running release.sh without -o. If one uses that switch, its most likely pointing to another dir.

Sure, doesn't really matter to me. I just like to eagerly exclude things out of habit.

dhewg commented 1 year ago

winetricks is by far the most used approach to install nine. It has a feature to fetch the latest nine release, so even future ones. Changing the paths of the files in the release tarball breaks that. So that's more important that making it WINEDLLPATH compatible, which you're probably the first user using it :)

But I guess adding a meson switch to enable create that newer layout should work for everybody?

9ary commented 1 year ago

It has a feature to fetch the latest nine release

Ah that makes sense, I didn't know. In that case, yeah, that's a breaking change. Let's see what I can do about it.

dhewg commented 1 year ago

Yeah, it's pretty neat since wintricks usually is a bit dated when installing it via distro packages, and that way you can still install the latest release with it.

It's the galliumnine verb, so without a fixed version suffix, which uses the github json api to get the lastest release: https://github.com/Winetricks/winetricks/blob/20230212/src/winetricks#L10239

9ary commented 1 year ago

I think this is the simplest. I just added hardlinks to the release archive so that existing winetricks releases can still work. Let's update winetricks to support the new structure, and then eventually this can be removed.

I see two causes of distros shipping an outdated copy of winetricks:

9ary commented 1 year ago

Hey @dhewg, it isn't urgent, but I just wanted to make sure you haven't forgotten about this PR. Thanks for putting up with me so far.

dhewg commented 1 year ago

Yeah, sorry for the delay. I merged the first four patches, I'm still not sure if the last two are worth the hassle. It's only a minor gain, which can be solved the other way around (create your own WINEDLLPATH and symlink files from your build dir in there).

dhewg commented 1 year ago

In fact I do something similar:

$ ls .wine/drive_c/windows/system32|grep nine
d3d9.dll -> /home/andre/.wine/dosdevices/c:/windows/system32/d3d9-nine.dll
d3d9-nine.dll -> /home/andre/games/nine/lib64/d3d9-nine.dll.so
ninewinecfg.exe -> /home/andre/games/nine/bin64/ninewinecfg.exe.so
9ary commented 1 year ago

To be honest, I don't really care what the binary release archive in this repo looks like. The main reason why I submitted this patch is because the current situation forces packagers to maintain their own install scripts separately, which is not ideal. Of course the files to install and their locations should be fairly stable, but it's still an extra burden for downstreams while fixing it here is a one-time annoyance. It's just easier to apply the same changes everywhere and then maintain backwards compatibility here until winetricks adapts.

The patch is already written, so there isn't any extra work to do here, but it's your call.

dhewg commented 1 year ago

I don't disagree in general, but this also breaks already present build scripts (users of --libdir/--bindir). And the symlinks for the old-style structure is only created after the fact in release.sh, which I assume most/all distro packages don't even use since distros have their own way to build things.

9ary commented 1 year ago

this also breaks already present build scripts (users of --libdir/--bindir).

This is fine. Packagers don't mind breaking changes to the build scripts as long as they are documented in the changelog. This is for example no worse than adding a new dependency.

They've already had to work around the breakage from Wine changes, which would not have been necessary if this had been fixed here from the start.

And the symlinks for the old-style structure is only created after the fact in release.sh, which I assume most/all distro packages don't even use since distros have their own way to build things.

Indeed, distros are expected to use ninja install and be done. I only added the links to the archive because you asked me not to break winetricks. I really don't like that it is downloading an unpinned release by default and without warning.

There are other cases where this happens in the wild, such as -git packages in the AUR, but users of those packages expect the breakage, and maintainers are usually quick to fix it.

dhewg commented 1 year ago

If a distro builds nine against the very wine version that distro is shipping there never was an issue. And I assume that is what all distros (which ship nine) are doing. It's our releases that we have to take care of, since we want to allow as many setups as possible for them to work on.

And as I mentioned, there is a reason winetricks allows to get future releases. That's an intentional feature and not related to distro packages blindly fetching git HEAD at all.

So let's try to not erroneously assign blame or compare apples to oranges.

9ary commented 1 year ago

If a distro builds nine against the very wine version that distro is shipping there never was an issue.

Yes, there was, see #123. The current build script does not install binaries to the correct location as of Wine 6.8 (released in 2021, 2 years ago). This led to distros such as Arch working around the problem by writing their own install script. I expect any distro shipping Wine 6.8 or newer would have had to do this. This is also independent of the Wine version being built against, because nine does not use Wine's build system.

And I assume that is what all distros (which ship nine) are doing.

The kind of distro that still ships Wine versions that are over 2 years old will not package a newer version of nine either, so we don't have to support them, packaging-wise.

It's our releases that we have to take care of, since we want to allow as many setups as possible for them to work on.

And my patch does not break any setup that you are targetting. It is merely making the job of downstream packagers easier. I wrote this patch as one such downstream, since I have https://github.com/NixOS/nixpkgs/pull/220853 pending (currently blocked on #155 since I don't want to ship a broken package; I had written a setup script which bypasses ninewinecfg but decided not to ship it).

And as I mentioned, there is a reason winetricks allows to get future releases. That's an intentional feature and not related to distro packages blindly fetching git HEAD at all.

So let's try to not erroneously assign blame or compare apples to oranges.

Technically speaking, it's the same thing. Keep in mind that I'm not necessarily questioning the reason why this is being done, but how it's being done. If winetricks is going to fetch an unpinned release, it should at least use the install script from the downloaded build rather than hardcode it. This would probably involve moving the winetricks install script into this repo.

I am comparing winetricks to packages that build from git HEAD for a reason: the latter have the luxury of responsive maintainers that will fix breakage within days. winetricks may be just as quick, but this means nothing if they don't tag releases (which only happens once or twice a year). You say winetricks downloads the latest release because distros ship old versions of it, but that's either:

dhewg commented 1 year ago

Yes, there was, see #123

I thought of another patch, obviously.

But still, nine was never designed to have a ninja install target which is compatible with your wine-nine level integration. The comments in #123 are rather clear in that regard.

Technically speaking, it's the same thing

Not in a sense that's relevant here, as we deliberately and knowingly chose this way. It wasn't an oversight or accident as the git HEAD example is.

But anyway, that doesn't mean we can't have an install target with a layout compatible with a wine-nine integration. We just have to make sure the current way keeps working, no matter if we like it or not. It may have been solved differently, but in the end it doesn't matter, because we now have to live with the way it was done years ago.

I'm just not fan of that mixed layout and symlinking in a post process script. Quoting myself from 2 weeks ago:

But I guess adding a meson switch to enable create that newer layout should work for everybody?

That would be nicer, why can't this install layout not be a build time decision, something like -Dwine-layout or whatever?

9ary commented 1 year ago

But still, nine was never designed to have a ninja install target which is compatible with your wine-nine level integration. The comments in #123 are rather clear in that regard.

That wasn't entirely clear to me, but now it is. Thanks.

Not in a sense that's relevant here, as we deliberately and knowingly chose this way. It wasn't an oversight or accident as the git HEAD example is.

But anyway, that doesn't mean we can't have an install target with a layout compatible with a wine-nine integration. We just have to make sure the current way keeps working, no matter if we like it or not. It may have been solved differently, but in the end it doesn't matter, because we now have to live with the way it was done years ago.

I've suggested an upgrade path, but I don't really want to take on the task. In the meantime I have no problem with maintaining compatibility with the existing system.

I'm just not fan of that mixed layout and symlinking in a post process script. Quoting myself from 2 weeks ago:

But I guess adding a meson switch to enable create that newer layout should work for everybody?

That would be nicer, why can't this install layout not be a build time decision, something like -Dwine-layout or whatever?

I felt that this would have been more complicated, and it would become a burden for future maintenance and contributions, rather than encouraging migration. In any case, backwards compatibility would have to be maintained until the next Debian release at the very least (I think latest Debian stable is a decent baseline for what to support).

I'm going to close this PR now and move this discussion to a new one, since I don't want to destroy history after the partial merge.