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

Misc cleanups #154

Closed
wants to merge 6 commits into from
Closed

Misc cleanups #154

wants to merge 6 commits into from

Conversation

9ary
Copy link
Contributor

@9ary 9ary commented Mar 18, 2023

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

@9ary 9ary force-pushed the cleanup branch 2 times, most recently from c862f3e to 5e01acb Compare March 18, 2023 15:39
@9ary
Copy link
Contributor Author

9ary commented Mar 18, 2023

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
Copy link
Collaborator

dhewg commented Mar 18, 2023

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

https://github.com/dhewg/wine-nine-standalone/commits/22.04
BUT: #146 (comment)
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
Copy link
Contributor Author

9ary commented Mar 18, 2023

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
Copy link
Contributor Author

9ary commented Mar 18, 2023

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
Copy link
Contributor Author

9ary commented Mar 18, 2023

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

@dhewg
Copy link
Collaborator

dhewg commented Mar 27, 2023

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
Copy link
Contributor Author

9ary commented Mar 27, 2023

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
Copy link
Collaborator

dhewg commented Mar 27, 2023

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
Copy link
Contributor Author

9ary commented Mar 27, 2023

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
Copy link
Collaborator

dhewg commented Mar 27, 2023

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

Fake DLLs contain no code and export no symbols, so passing any code
objects to the linker is entirely pointless.

This also fixes a meson warning about an extract_all_objects() change.
Not only is this friendlier to packagers, but it enables usage via
WINEDLLPATH.

Fixes iXit#123
winetricks can download future releases that it doesn't package yet.

Because it doesn't use our install script, we need to preserve the
existing structure in our release archive.

Let's just link the files back to the right place until downstreams
catch up.
@9ary
Copy link
Contributor Author

9ary commented Mar 27, 2023

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:

  • winetricks maintainers rarely tag releases, this is 100% their fault.
  • Distros like Debian stable that simply don't update for years. I don't think we can do much about this from our end, we should just let them deal with the breakage by patching that behavior out (probably best for them) or backporting the real fix. Or just wait until the next Debian release that includes a fixed winetricks to remove BC from here.

@9ary
Copy link
Contributor Author

9ary commented Apr 12, 2023

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
Copy link
Collaborator

dhewg commented Apr 12, 2023

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
Copy link
Collaborator

dhewg commented Apr 12, 2023

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
Copy link
Contributor Author

9ary commented Apr 12, 2023

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
Copy link
Collaborator

dhewg commented Apr 12, 2023

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
Copy link
Contributor Author

9ary commented Apr 12, 2023

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
Copy link
Collaborator

dhewg commented Apr 12, 2023

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
Copy link
Contributor Author

9ary commented Apr 12, 2023

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 NixOS/nixpkgs#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:

  • intentional; "stable" distros pin packages for a long time by design, and I would argue that subverting this is not a good idea
  • upstream's fault; again, winetricks does not tag releases often enough for changes to reach users, when it looks like git HEAD is the version that is intended for use

@dhewg
Copy link
Collaborator

dhewg commented Apr 12, 2023

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
Copy link
Contributor Author

9ary commented Apr 12, 2023

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.

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.

2 participants