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

edopro: init #342365

Merged
merged 1 commit into from
Oct 23, 2024
Merged

edopro: init #342365

merged 1 commit into from
Oct 23, 2024

Conversation

Redhawk18
Copy link
Contributor

@Redhawk18 Redhawk18 commented Sep 16, 2024

Description of changes

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

TODO

  • card assets
  • sound
  • controller support
  • core (statically) linked for runtime use
  • update script
  • optional: card asset discussion (edo9300 gives permission to us)
  • optional: font discussion (unneeded for now)
  • optional: cross platform discussion (no macs to test on)
  • optional: windbot support (I believe it works)

@TLATER

@Redhawk18
Copy link
Contributor Author

I should note that @TLATER wrote the init for this package and I just carried it to the finish line.

Copy link
Contributor

@TLATER TLATER left a 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.


if [ ! -d $EDOPRO_DIR ]; then
mkdir -p $EDOPRO_DIR
cp -r ${assets.src}/{${assetsToCopy}} $EDOPRO_DIR
Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor Author

@Redhawk18 Redhawk18 Sep 18, 2024

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.

pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
@Redhawk18
Copy link
Contributor Author

In other news I got sound working!

@Redhawk18
Copy link
Contributor Author

Redhawk18 commented Sep 17, 2024

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.

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 by-name will help fix this issue because any actual code updates could be pulled into nixos unstable as fast as a week with the new auto update and auto merge bots that are working on nixpkgs. You forget the downside of using nixld is we lose all ability to actually debug this project if something goes wrong. For example when I was trying to figure out the card asset urls, no where in any doc or build file does it say the it needs the link followed by a formatter like {}.jpg. You would need to use the strings command on the binary to figure that part out.

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 nix-build -A edopro after updating the hashes to build the newest version from git to debug, not have to install all the sdk's and runtimes manually. Packaging would help us, the users of nixos.

@TLATER
Copy link
Contributor

TLATER commented Sep 17, 2024

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.

@winterqt winterqt marked this pull request as draft September 17, 2024 22:21
@winterqt
Copy link
Member

Marking as draft due to eval failures; please feel free to undraft once these have been fixed.

Copy link
Contributor

@TLATER TLATER left a 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 Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved

if [ ! -d $EDOPRO_DIR ]; then
mkdir -p $EDOPRO_DIR
cp -r ${assets.src}/{${assetsToCopy}} $EDOPRO_DIR
Copy link
Contributor

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.

pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
@Redhawk18
Copy link
Contributor Author

Marking as draft due to eval failures; please feel free to undraft once these have been fixed.

what? who failed to eval, it works on my system and is there a log?

@winterqt
Copy link
Member

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".

@Redhawk18
Copy link
Contributor Author

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.

Wrong! this is building from the tagged releases! :)

@Redhawk18
Copy link
Contributor Author

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".

what is restricted mode and why can't it download from github?

pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
@Redhawk18
Copy link
Contributor Author

note we have to use fmt 9 until we get to this commit

1108eab5e7b3b3bda2a8d6a42e5f25ae4d3292fa

@Redhawk18 Redhawk18 force-pushed the 3dopolololo branch 2 times, most recently from 7285774 to 7bcabcb Compare September 18, 2024 19:09
@Redhawk18
Copy link
Contributor Author

I got windbot working!

@Redhawk18
Copy link
Contributor Author

@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.

@Redhawk18
Copy link
Contributor Author

@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.

@nixos-discourse
Copy link

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

@Redhawk18 Redhawk18 force-pushed the 3dopolololo branch 3 times, most recently from 42055fe to 3afc49b Compare October 17, 2024 21:34
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
@Redhawk18 Redhawk18 force-pushed the 3dopolololo branch 2 times, most recently from 8448bf4 to 5439c14 Compare October 21, 2024 17:34
@Redhawk18
Copy link
Contributor Author

@OPNA2608 Hm, not sure why vet is failing now. All I added was your suggestion code and patchfile

@OPNA2608
Copy link
Contributor

#350289 (comment)

@Redhawk18
Copy link
Contributor Author

Redhawk18 commented Oct 21, 2024

#350289 (comment)

now thats epic, and please approve

pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/ed/edopro/package.nix Show resolved Hide resolved
@Redhawk18
Copy link
Contributor Author

Is there anything else?

@OPNA2608
Copy link
Contributor

Please format with nixfmt like CI says.

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise LGTM, sans the nixfmt CI error.

pkgs/by-name/ed/edopro/package.nix Outdated Show resolved Hide resolved
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 <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Update pkgs/by-name/ed/edopro/package.nix

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Apply suggestions from code review

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

change hash format

Apply suggestions from code review

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

Apply suggestions from code review

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

add patchfile and multiarch function

Apply suggestions from code review

Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>

add deps format versioning

fmt
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/2057

@nixos-discourse
Copy link

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/4731

@OPNA2608 OPNA2608 dismissed winterqt’s stale review October 23, 2024 15:50

Request was addressed.

@OPNA2608 OPNA2608 merged commit 2b2950f into NixOS:master Oct 23, 2024
11 of 12 checks passed
@Redhawk18 Redhawk18 deleted the 3dopolololo branch October 23, 2024 16:19
@Redhawk18
Copy link
Contributor Author

Now this is epic

peterhoeg pushed a commit to peterhoeg/nixpkgs that referenced this pull request Oct 24, 2024
Co-authored-by: Cosima Neidahl <opna2608@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants