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

user install option broken since #140 #148

Closed
db47h opened this issue Jul 14, 2024 · 17 comments · Fixed by #174
Closed

user install option broken since #140 #148

db47h opened this issue Jul 14, 2024 · 17 comments · Fixed by #174

Comments

@db47h
Copy link
Contributor

db47h commented Jul 14, 2024

Since #140, make install fails trying to install umu_version.json in under /usr instead of ~/.local:

:: Installing umu_version.json 
install -d /usr/lib/python3.12/site-packages/umu-launcher
install: cannot change permissions of ‘/usr/lib/python3.12/site-packages/umu-launcher’: No such file or directory
make: *** [Makefile.in:60: version-install] Error 1

Fixing PYTHONDIR in Makefile.in is not sufficient. Also, installing python modules in ~/.local/lib is no longer permitted on some systems (see PEP 668). As a side note: this variable causes issues for packagers since it's hard-wired to the system wide python module path of the build environment.

I have a tentative fix here: db47h@38f5065 that involves two things:

  1. have python build/install handle umu_version.json instead of manually installing it
  2. use pipx for user installs. We could also use pip install --target ... but this would require fiddling with the PYTHONPATH.

See the commit log for more details. Please let me know if this is an acceptable solution, if so, i'll update the docs and configure.sh before submitting a PR.

@db47h db47h changed the title user install option broken since e52f88d12e9bba71e728e01bea6edb58e6fa3ee3 user install option broken since #140 Jul 14, 2024
@R1kaB3rN
Copy link
Member

For me, I guess my only concern for user installations is for users being out of date because there's no self-update functionality. Unless some self-update functionality is implemented, I think users should look into using the Flatpak for a user install or look into using a system package. Or we can continue to support user installs, but have umu as a zipapp installed in $HOME/.local/bin, which I think I would be fine with as long as its reproducible and there's no problems it.

cc @loathingKernel for thoughts on supporting user installs.

In any case, since that option is broken, it needs to be removed from the README.

R1kaB3rN added a commit that referenced this issue Jul 18, 2024
- This option is currently broken, so it shouldn't be listed anymore.

- Related to #148
@db47h
Copy link
Contributor Author

db47h commented Jul 18, 2024

I think the zipapp option is great (#149 ?), both for users and front-ends. While front-ends should handle updates themselves, self update for users should be doable with a zipapp. Either by rebuilding or pulling a prebuilt zipapp from github releases.

@loathingKernel
Copy link
Contributor

loathingKernel commented Jul 18, 2024

So, general thoughts on this along with some related packaging concerns,

First and foremost, I am on the fence about the user-install option now that we have a flatpak, a snap and some distribution packages, but we should not make it hard or complicated to install it as a user either in a virtual environment or in some other form of python packaging (zipapp, pyinstaller, w/e).

  • Do we still need version.json? The reason we added it was to update the user files when we used to copy them. Right now the only important thing it tracks is the SteamLinuxRuntime version and even that is a generic name and umu pulls in the latest SLR.
    • If for some reason we do want to keep some form of it, we should leverage either some hatchling functionality, although I haven't looked if it offers something relevant, or setuptools_scm to generate a version file through the python build system. This should remove the circular dependency we have right now that the makefile generates the version, then hatchling packages through the makefile.
  • Taking care of the above should make umu distributable and installable through PyPI, which partially takes care of some aspects of user-local installation. It should also make the makefile less strange, as it would remove the need for PYTHONDIR (umu-launcher compat tool should call the executable in each case) and the shebang fixes.
  • None of these options take care the umu-launcher compatibility tool installation, should we retain it for system packages only? Remove it? We could always revert back to having umu copy those files in-place to the user compatibilitytools.d (we don't do that for flatpak either, do we?)
  • Packages with vendored dependencies can be facilitated with one of the many methods that exist for Python mentioned above, if we make umu install the compatibility tool itself, taking care of that aspect. The makefile should then use such a method to create a standalone package.

@db47h
Copy link
Contributor Author

db47h commented Jul 18, 2024

  • If for some reason we do want to keep some form of it, we should leverage either some hatchling functionality

Hatchling can bundle the version file automatically: add umu_version.json to artifacts in pyproject.toml:

[tool.hatch.build.targets.wheel]
# ...
artifacts = [
  "umu/umu_version.json",
]

In the makefile, the version target needs to stay but version_install can go. I haven't looked further, like having it automatically generated though. On the other hand, getting rid of that file would make sense: if an update of the steam runtime is needed, an update of umu is probably needed too.

  • Taking care of the above should make umu distributable and installable through PyPI

That would be ideal.

EDIT and side note on packaging: Users need to be able to update any piece of software related to gaming support, like wine or umu, as often as needed. At least more often than the average Linux distro's 6 months release cycle. So, as a user, I would never use a system package for this, but either rely on heroic/lutris to keep it updated or use flatpak. Not snap either, too much overhead for a 1MB python package.

@db47h
Copy link
Contributor Author

db47h commented Jul 18, 2024

nope, it doesn't generate the file. I'll look into it if you want.

@loathingKernel
Copy link
Contributor

I was wondering if hatchling could generate version information. Including arbitrary files is trivial. My goal would be to remove the version generation from the makefile altogether to have no dependency on it for the python part of umu. setuptools_scm can do that to a certain extend if we deem the version important to stay.

@db47h
Copy link
Contributor Author

db47h commented Jul 18, 2024

For umu's purpose, I think we could use this:
https://hatch.pypa.io/latest/config/build/#build-hooks
with the built-in custom hook. No need for additional dependencies. I'll try and put something together later today (UTC+1 timezone).

EDIT: want to keep the json or throw it away and bake-in umu's version info at build time? Steam runtime version can be set as a const somewhere.

@loathingKernel
Copy link
Contributor

Let's see what @R1kaB3rN has to say because this is not a change pertaining the makefile only.

@R1kaB3rN
Copy link
Member

Yeah, I agree with loathingkernel's points about the build system. The current state of the Makefile is a bit strange, and we can delegate a lot of the work to hatch (e.g., installing the version file and generating the version) which would simplify things.

Let's keep the JSON file for now. Besides managing the launcher/runtime files, another reason why umu_version.json existed was because Lutris was interested in keeping track of umu-launcher builds for their own tracking/reporting purposes. While they pull from the latest Github release by default, they implement client logic to use either the system installed umu or the one provided by their runtime. So having an easy way to identify the build used in a session would be helpful for them as well as for umu maintainers when it comes to reporting/debugging.

@R1kaB3rN
Copy link
Member

As for umu-launcher, it would be nice to have a hatch hook to manage this as well. And for now, I would prefer it to remain as system package property. As far as I know, only Rare cares about this and we can revisit having it be copied again for user installs once we start building/customizing the runtime or when there are more popular use cases to enable it (e.g., potential decky loader plugin?).

@loathingKernel
Copy link
Contributor

loathingKernel commented Jul 19, 2024

As far as I know, only Rare cares about this and we can revisit having it be copied again for user installs once we start building/customizing the runtime

It's true that only Rare uses this as far as we know right now, although that might change, since it doesn't do presently what I would like it to do. For example, having the umu flatpak installed, if we don't install the compatibility tool through umu, Rare won't find it, so I will either have to handle that case in Rare, making the compat tool not useful for Rare either. Personally I would prefer it for umu to install those files itself, making the tool available in all installation methods, pip/zipapp included.

If you are busy with other things, I could figure the specifics out (mostly path related stuff, since different install methods would have to be executed in different ways) and PR them later the following week.

@db47h
Copy link
Contributor Author

db47h commented Jul 19, 2024

Back to version autogen, I ended up using hatch-vcs. I don't like adding another build dependency, but it's the less painful solution. Here's what the changes to pyproject.toml look like:

diff --git a/pyproject.toml b/pyproject.toml
index 1e3d7a4..327a2a8 100644
--- a/pyproject.toml
+++ b/pyproject.toml
@@ -1,5 +1,5 @@
 [build-system]
-requires = ["hatchling"]
+requires = ["hatchling", "hatch-vcs"]
 build-backend = "hatchling.build"
 
 [project]
@@ -42,7 +42,11 @@ dependencies = ["python-xlib>=0.33"]
 umu-run = "umu.__main__:main"
 
 [tool.hatch.version]
-path = "umu/__init__.py"
+source = "vcs"
+
+[tool.hatch.build.hooks.vcs]
+version-file = "umu/umu_version.json"
+template = "{ \"umu\": { \"versions\": { \"launcher\": \"{version}\", \"runner\": \"{version}\", \"runtime_platform\": \"sniper\" } } } } } }"
 
 [tool.hatch.build.targets.sdist]
 exclude = [
@@ -64,6 +68,9 @@ exclude = [
   "/umu/umu_test.py",
   "/umu/umu_test_plugins.py",
 ]
+artifacts = [
+  "umu/umu_version.json",
+]
 
 [tool.mypy]
 python_version = "3.10"

It's still not working because of weird things happening in the template, but when I eventually get it to work, it will generate the umu_version.json, and bundle it, all using python -m build.

@loathingKernel
Copy link
Contributor

I approve of this solution personally. Using a plugin to the build system is fine IMHO.

@db47h
Copy link
Contributor Author

db47h commented Jul 19, 2024

Just pushed #154. I did not remove umu/umu_version.json from the clean rule since it might still have its use for testing purposes and it won't break anything anyway. I'll update #149 accordingly.

@Ketrel
Copy link

Ketrel commented Aug 16, 2024

I just wanted to clarify, is there an intent to fix user installs or is that permanently no longer supported?

@R1kaB3rN
Copy link
Member

Yes, there's still an intent to fix this problem. Like I said, I think it's currently fine to have a zipapp as long as it's reproducible. I think it would be good to support as it'll have an advantage over the Flatpak/Snap package in terms of both build time/install size, and it adds the least amount of complexity compared to those or similar packaging solutions.

One thing to keep in mind though is its limitation. While this is not relevant now, if the launcher ever decides to import non-Python modules, I believe those will be need to be outside the zipapp. At that point, I think we would be back to the beginning, and would have to not support zipapp anymore.

@alterNERDtive
Copy link

🥳

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 a pull request may close this issue.

5 participants