edopro: 40.1.4 -> 41.0.2 #400930
edopro: 40.1.4 -> 41.0.2 #400930
Conversation
github-actions
bot
added
10.rebuild-darwin: 0
idk man, this is kinda risky. Remember what we talked about before? The reason that engine isn't versioned is because its an unstable rolling release. The only way to pin the version correctly is to have a bot like nixpkgs upkeep run the script every week to check for update. |
|
The next time you update this package it won't compile because the engine will be outdated. |
|
Okay I built it in x64 and it does compile, but theres no core? Was there a pr that went through with me getting pinged? |
IIRC that was about ocgcore getting bundled, which this isn't touching. This is unlocking the pinning of the irrlicht rendering engine from a fixed 2yo Git revision to "whatever is the latest commit when an edopro update is found".
Or am I misremembering things?
The time I tried to update the package, it wouldn't compile because irrlicht was outdated.
AFAIK, the only meaningful change was c868f57. But that happened in a PR to staging, and gets more-or-less reverted in this PR.
I only have aarch64 hardware left right now, for which there is no core available anyway, so unable to test this part. Assuming you waited abit to make sure the core doesn't just get loaded late (always took afew secs or so on my machine):
|
I disagree, we are taking a package that was automatic and adding manual requirements to updating it again. We should use a bot like @r-ryantm to update the package automatically. If I enjoyed updating packages manually I wouldn't have wrote the update script. I just forgot to add the bot.
I have no idea, I compared the 24.11 version to this pr and checked everything and I can't figure out the problem but I am sure it doesn't work. |
What "manual requirement" are you talking about? Currently, there is a manual requirement (irrlicht rev pin inside of the script) which I'm removing to make it more automatic (latest irrlicht whenever new edopro tag is found). I invite you to run the update on current master via standard means (
@r-ryantm runs the script on a schedule. It will run into the aforementioned build error, and should thus never open a bump PR on its own, because it (correctly) assumes that the update is borked.
You'll need to keep up with upstream releases anyway, whether by following the repo or by periodically running the
You cannot "add" the bot. The bot runs automatically. It runs
|
OOPS MY BAD LMAO. I pinned that for testing and forgot to remove it. I was confused because I knew I wrote the logic you're describing. |
|
The last problem then is that the core doesn't work. It is there, I checked through dolphin and it is statically linked. |
|
No problem! So:
New version, new repo: Presumably, new libraries only get uploaded there to override the bundled ocgcore from upstream's downloads. So we prolly do need to build an ocgcore library, to provide a base core in situations like this. We just can't build edopro with the assumption that it will only use that core, like we tried & failed to in the past, because eventually DeltaBagooska will have ocgcore libraries, and script updates will require a newer core. So we still wouldn't need to keep up with upstream's core updates, we just need to provide a base version and configure the game to check the repo for a newer version like it does right now. I don't have a solution for that right now, but I'll play around abit. Marking this a draft, since without a core there's no singleplayer functionality. |
|
Thats a fair point, this is a sticky issue because typically you don't want to have pr's that look like |
- Error output was not getting printed to stderr, so running it via update.nix wouldn't display the error - Fetched latest irrlicht was getting overridden by fixed irrlicht rev, meaning that irrlicht would never get an update
Unlike last time, we still build edopro to look in downloaded script repos for updated ocgcores. We just patch it so it can find & load the ocgcore from its release tag first, in case the scripts download doesn't have a core.
|
I like this, as I understand we packaged a fallback core but we still allow for downloaded cores? In case wouldn't we still require lua in edopro?
|
The new version wouldn't actually clone Maybe this should be done for some of the other files as well, like the This might need a deeper look into how we should handle this.
Correct. There is no upstream option for this exact use case (load base ocgcore from a specific location, without disabling loading of updated cores from repo downloads), so I had to patch the location where the base ocgcore is looked up.
Yes, ocgcore still needs Lua. The latest version (which the submodule from edopro points at) builds its own Lua from a submodule. We can tell it to build against our Lua again (
The script repo will have multiple ocgcore builds for different platforms. edopro version 41 added support for aarch64-linux, so I would expect them to have adjusted the corresponding code to load the correct ocgcore builds. Once one gets pushed to the repo, I can let you know if it loads on my end. Otherwise, loading the new core would just fail, and it would stick with the base core it has already loaded. If it hasn't loaded a base ocgcore yet, then it will continue with no core loaded. This is the current behaviour on aarch64-linux, since the version on master didn't yet have aarch64-linux builds in its update downloads.
If the base ocgcore is wrong, it should just fail to load. But the core is built from the ocgcore commit that the corresponding edopro tag points to in its submodule - if that's completely invalid for the built edopro version, to the point where it refuses to be loaded (excluding linking errors, which are prolly to be fixed on our end first), then that's prolly worth reporting upstream. I haven't been able to actually test if the core actually works yet, I've only checked via I'm currently recovering from a surgery, so expect further work from me on this to take abit. |
|
I will look into this tonight, before when I was testing I was only deleting the repo directory, the the config file you are mentioning. |
|
Okay I did some testing, this works. I'm not the biggest fan of the updating process tbh. Because I tried to delete just the |
|
Here's an idea for solving the update "which files to sync" issue: Just don't bother. Let's put the assets into versioned sub-directories, check if there are >1 directories in
This way, every version will have its correct base assets, and there shouldn't be any conflicts between them. The pop-up is also just informational, and will not block the user from just ignoring the situation if they wish/need to keep stuff unmigrated and just want to continue to the game. I don't think this is optimal, but it should be easier than keeping track of which files need to match the currently running version for everything to work, and dealing with cross-version breakages. If you like the idea, I can submit what I have and we can start bikeshedding about any of the phrasing etc. 🙂 DeltaBagooska has cores now, and it loads the aarch64 one from it fine... But I noticed that it can't find the fallback one on aarch64. I'll adjust some stuff to fix this when I have time. |
|
HUH, we can do this??? I didn't think we had the ability to do this type of stuff. |
… subdirs - Use WriteShellApplication for the launcher script, since that provides some good default shell options and runs shellcheck on the code - Fix any issues reported by shellcheck - Keep assets in subdirs with corresponding edopro version Since some of the assets tell the game which game scripts to download, where to look for them, and which servers to look at for online play, and managing this across versions could be messy - Check for multiple subdirs and issue warning about migrating data Let's not try to solve this ourselves, the user will know best if, and what, they care about when switching between versions
|
"Versioned" was prolly the wrong word for describing this previously, since that implies smth like a Submitted the change, please have a look. I suppose the message could be abit better, since the initial migration would be from a version-less Core loading is happy on aarch64 now, I hope I didn't miss anything necessary on basic x86_64-linux. Since upstream has defines for platforms we technically know as targets in Nixpkgs, I've added optimistic handling for those. Obviously untested though. |
- Rename ocgcore library to match expected platform-specific name
| # where to get incremental updates from, online servers, card scripting, certificates for communication etc), | ||
| # and some are optional nice-haves (example decks). It's also possible to override assets with custom skins. | ||
| # | ||
| # Don't try to manage all of this across versions, just inform the user that they may need to migrate their |
There was a problem hiding this comment.
Why don't we handle it, why did we give up so soon?
There was a problem hiding this comment.
Simplicity, being able to eventually get a version of these bumps in, lack of spoons to fully figure this out any time soon.
If you want to figure out which files we need to keep synced, and the best way of syncing these without overwriting user-touched files, feel free to send a patch for me to apply.
|
I've been monitoring this PR and the update is working well for me too. Nice work on the versioned subfolders and the ocgcore fix. I see this is still drafted but feel free to undraft and/or ping me if you need a merge. |
|
@r-burns Undrafted now. Have the honour of pressing the merge button 😄. |
|
Oh lol, didn't realize you were a committer yourself, I'll go ahead and merge anyway |
|
Successfully created backport PR for |
3 participants
Bagoosker
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.