-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
edopro: init #342365
base: master
Are you sure you want to change the base?
edopro: init #342365
Conversation
I should note that @TLATER wrote the init for this package and I just carried it to the finish line. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed elsewhere, I've previously tried getting a package together for this, but this is one of those applications that don't play particularly nicely with NixOS. The game assets are deliberately mutable, and the concept of "version" is muddy.
Upstream also very strongly suggested that third party builds will just cause headaches and that we should instead suggest using nix-ld
with their published binaries, which works just fine. My build primarily exists because I wanted to make some downstream changes.
I think we may want to just wrap that binary in an fhs env with a little script to start it in a appropriate xdg base dir.
pkgs/by-name/ed/edopro/package.nix
Outdated
|
||
if [ ! -d $EDOPRO_DIR ]; then | ||
mkdir -p $EDOPRO_DIR | ||
cp -r ${assets.src}/{${assetsToCopy}} $EDOPRO_DIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't quite ready for general use, ideally this should have some logic that prevents overwriting user edits but recovers the directory if it's broken. Also applying updates would be difficult with the current script, and will probably become important eventually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You wrote this code, I've made no changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, which is why I point it out as a remaining problem ;p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can think of a better way to do this sometime this week, don't have that much disposable evening time currently, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked edopro about this and he confirmed the files we need to never override if they exist they are.
- config/user_configs.json
- deck
- pics
- replay
Everything else we can nuke on an update.
In other news I got sound working! |
We can add some more checks but at the end of the day these fights always happen. Most commonly you'll see them in fast moving projects like emulators, by having this project be in We can also add the update url if that helps, I believed the update url meant a "update is available, download here" which would just confuse most users. But if handles that and card assets for example we could add it back. I believe it's important to remember context here, edopro has open pr's from almost 5 years ago that have gone stale, in contrast nixos's autoupdate pr merger will be able to run laps around the speed required to keep this app up to date without any problems on their dev team. In the worse possible case all a user would have to do is run |
e5652ca
to
3c95502
Compare
The issues we'll run into are dependency compatibility, precisely because edopro updates slowly. This is building directly from the master branch, which is not an actual released version; the released version is incompatible with current gcc, for example, and we may well run into version incompatibility with the official servers in the future. |
Marking as draft due to eval failures; please feel free to undraft once these have been fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At a computer now; I point out a couple more polish things.
Final discussion point:
optional: windbot support
Would be nice indeed, but probably a nightmare to build as well: https://github.com/IceYGO/windbot
We could bundle what comes with the edopro asset release, but that one has actual dependencies, unlike libocgcore
, so you'd need an fhs env/nix-ld at that point.
Probably something to mark down as future work.
pkgs/by-name/ed/edopro/package.nix
Outdated
|
||
if [ ! -d $EDOPRO_DIR ]; then | ||
mkdir -p $EDOPRO_DIR | ||
cp -r ${assets.src}/{${assetsToCopy}} $EDOPRO_DIR |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll see if I can think of a better way to do this sometime this week, don't have that much disposable evening time currently, though.
what? who failed to eval, it works on my system and is there a log? |
OfBorg, our CI, did. See the X under "ofborg-eval". |
Wrong! this is building from the tagged releases! :) |
what is restricted mode and why can't it download from github? |
3c95502
to
93d2862
Compare
93d2862
to
a83bad3
Compare
note we have to use fmt 9 until we get to this commit |
7285774
to
7bcabcb
Compare
I got windbot working! |
7bcabcb
to
2599d2a
Compare
@TLATER I got the main body of the update script done, it's not finished but I can't find a way to set the the hashs required for nix. Since somehow we need to invoke nix's eval, but I'm unsure how. |
@TLATER hey man, just checking on you. We might just come back to the update script later, it's almost done I just need to figure out the hashes. |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/how-to-get-the-proper-hashes-from-a-github-repo-and-commit/54460/1 |
42055fe
to
3afc49b
Compare
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/4702 |
@TLATER It's ready for review if you have time. |
3afc49b
to
ec03ef3
Compare
1b9d960
to
79b84a1
Compare
79b84a1
to
16cd0e4
Compare
4abf674
to
bf2c280
Compare
438674e
to
8448bf4
Compare
fmt sound suggestions 1 license update add support for windbot escape string xdg directory with a fallback of $HOME add update script and new deps file finished update script format deps file remove testing logic remove tlater, because he is not in the maintainer list and didn't response to me Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Update pkgs/by-name/ed/edopro/package.nix Co-authored-by: Cosima Neidahl <[email protected]> Apply suggestions from code review Co-authored-by: Cosima Neidahl <[email protected]> change hash format Apply suggestions from code review Co-authored-by: Cosima Neidahl <[email protected]> Apply suggestions from code review Co-authored-by: Cosima Neidahl <[email protected]> add patchfile and multiarch function
8448bf4
to
5439c14
Compare
@OPNA2608 Hm, not sure why vet is failing now. All I added was your suggestion code and patchfile |
now thats epic, and please approve |
|
||
irrlicht-edopro = stdenv.mkDerivation { | ||
pname = "irrlicht-edopro"; | ||
version = "1.9-custom"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.9-custom
doesn't really identify a version in any way that suits the version
attribute. Would it be possible to have this be generated in our unstable version format 0-unstable-<YYYY>-<MM>-<DD>
as part of your update script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you look at the project 1.9.0 is the version in the project.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1.9.0
doesn't name a specific rev either. The version is inherited from upstream Irrlicht, where it's been used as the "upcoming next minor release" for years.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like is stated in the update script, irrlicht has no update structure. It's released as a rolling release, which is why I try to exit the update script before updating hashes.
|
||
ocgcore = stdenv.mkDerivation rec { | ||
pname = "ygopro-core"; | ||
version = deps.ocgcore-rev; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar note here, except that this follows our version guideline even less than irrlicht-edopro
- "4a872ad7686e0d00ee4c1c051e90fc07c40613d4"
.
Currently this points at edo9300/ygopro-core@4a872ad, which would be 9.1-unstable-2022-09-15
. There was a release 2 days after that commit which would make it a clean 10.0
, but dunno if they didn't bump to that on purpose (maybe worth a ping upstream?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a git module so the context of versions doesn't really make sense here. Even if I update the script to fetch the commit and see if it has a related tag its not always going to resolve correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I pinged him anyways
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How on earth do you want my script to get the version this core is from?
Co-authored-by: Cosima Neidahl <[email protected]>
Description of changes
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.
TODO
@TLATER