Skip to content

godot3: refactor, rename from godot, and add mono builds#177578

Merged
lilyinstarlight merged 3 commits intoNixOS:masterfrom
Rotaerk:godot-mono
Aug 18, 2023
Merged

godot3: refactor, rename from godot, and add mono builds#177578
lilyinstarlight merged 3 commits intoNixOS:masterfrom
Rotaerk:godot-mono

Conversation

@Rotaerk
Copy link
Contributor

@Rotaerk Rotaerk commented Jun 13, 2022 •
edited
Loading

Description of changes

Refactored to reduce repetition of common installation steps across the different derivation variations, improving the granularity of overridability. This helped with the next step: Adding new derivations for the Mono version of Godot.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.11 Release Notes (or backporting 22.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jun 13, 2022 •
edited
Loading

I haven't really done this before, so not clear on what I might be missing. Should I pre-squash my commits, or perhaps rewrite history in a more tidy way? My second commit was a refactor with some false starts that were revised in the third commit ... My fourth is a minor correction.
I rewrote history such that the commits are more tidy and fit the CONTRIBUTING.md guide.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jun 13, 2022
@ofborg ofborg bot requested review from 90-008 and jojosch June 13, 2022 23:08
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jun 13, 2022
@Rotaerk Rotaerk force-pushed the godot-mono branch 2 times, most recently from 67703b4 to cc6f876 Compare June 15, 2022 22:38
jojosch
jojosch reviewed Jun 16, 2022
View reviewed changes
Copy link
Member

@jojosch jojosch left a comment

Choose a reason for hiding this comment

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

Looks good so far - however, I'm personally not that involved in godot (in nixpkgs and in general). I only "duplicated" the server derivation to headless to be able to build oh-my-git (see #119642).

But oh-my-git can be built successfully with the changes. The godot editor also builds and launches a sample project.

Result of nixpkgs-review pr 177578 run on x86_64-linux 1

9 packages built:
  • godot
  • godot-export-templates
  • godot-headless
  • godot-mono
  • godot-mono-export-templates
  • godot-mono-headless
  • godot-mono-server
  • godot-server
  • oh-my-git

90-008
90-008 suggested changes Jun 16, 2022
View reviewed changes
Copy link
Member

@90-008 90-008 left a comment

Choose a reason for hiding this comment

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

I can't comment on the Mono parts much but it mostly looks fine to me... Just a few nitpicks 😛

@90-008
Copy link
Member

90-008 commented Jun 16, 2022

Result of nixpkgs-review pr 177578 run on x86_64-linux 1

9 packages built:
  • godot
  • godot-export-templates
  • godot-headless
  • godot-mono
  • godot-mono-export-templates
  • godot-mono-headless
  • godot-mono-server
  • godot-server
  • oh-my-git

@ofborg ofborg bot requested review from 90-008 and jojosch June 17, 2022 06:30
90-008
90-008 suggested changes Jun 17, 2022
View reviewed changes
@ofborg ofborg bot requested a review from 90-008 June 18, 2022 07:18
90-008
90-008 approved these changes Jun 18, 2022
View reviewed changes
Copy link
Member

@90-008 90-008 left a comment

Choose a reason for hiding this comment

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

LGTM

SuperSandro2000
SuperSandro2000 requested changes Jun 19, 2022
View reviewed changes
Copy link
Member

@SuperSandro2000 SuperSandro2000 left a comment

Choose a reason for hiding this comment

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

I don't think we can merge this PR in the current state. The proposed changes are overly complex and complicated and can be greatly simplified.

SuperSandro2000
SuperSandro2000 reviewed Jun 20, 2022
View reviewed changes
SuperSandro2000
SuperSandro2000 reviewed Jun 20, 2022
View reviewed changes
@bobby285271 bobby285271 added 12.approvals: 1 This PR was reviewed and approved by one person. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages. labels Jun 23, 2022
@Rotaerk Rotaerk requested a review from SuperSandro2000 June 23, 2022 16:56
@ofborg ofborg bot requested a review from 90-008 June 23, 2022 17:03
@ofborg ofborg bot added the 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. label Jun 23, 2022
@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 16, 2023

That seems like a good idea; how do you do that?

@Janik-Haag
Copy link
Member

Janik-Haag commented Jul 16, 2023 •
edited
Loading

look at pkgs/top-level/aliases.nix for examples that's also the place where you would add new ones.

@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 16, 2023

Thanks, never noticed aliases.nix. Added aliases for the old names.

@ofborg ofborg bot requested a review from Twey July 16, 2023 20:49
lilyinstarlight
lilyinstarlight reviewed Jul 16, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

I think this is pretty close to the finish line, thank you for all this work!

I left a few questions below, but pending answers on those I don't see any other blockers for merge

@Janik-Haag Janik-Haag mentioned this pull request Jul 17, 2023
Closed
12 tasks
@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 18, 2023

Pushed a new version that just adjusts the sh ifs to use -n, and uses git rev-parse --show-toplevel to figure out the path within make-deps.sh. The patchelf idea didn't seem to pan out, but if there are any other ideas in that vein, I can try them out.

ghost
ghost reviewed Jul 22, 2023
View reviewed changes
@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 27, 2023

This push eliminates the detect.py patches and switches to autoPatchelfHook-based corrections to add udev, pulseaudio, and alsa as runtime dependencies. Also renamed the man page file to godot3.6 so that godot 4 can have a distinct man page.

@Janik-Haag
Copy link
Member

Janik-Haag commented Jul 27, 2023

Result of nixpkgs-review pr 177578 run on x86_64-linux 1

19 packages failed to build:
  • gdtoolkit
  • gdtoolkit.dist
  • godot3-export-templates
  • godot3-headless
  • godot3-headless.dev
  • godot3-headless.man
  • godot3-mono
  • godot3-mono-debug-server
  • godot3-mono-export-templates
  • godot3-mono-headless
  • godot3-mono-headless.dev
  • godot3-mono-headless.man
  • godot3-mono-server
  • godot3-mono.dev
  • godot3-mono.man
  • godot3-server
  • lorien
  • oh-my-git
  • pixelorama
4 packages built:
  • godot3
  • godot3-debug-server
  • godot3.dev
  • godot3.man

@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 27, 2023

huh. they built locally ... I'll check tomorrow.

@Janik-Haag
Copy link
Member

Janik-Haag commented Jul 27, 2023

huh. they built locally ... I'll check tomorrow.

the build server restarted while it was building, gonna build it again later ^^ don't worry D:

@Janik-Haag
Copy link
Member

Janik-Haag commented Jul 27, 2023

Result of nixpkgs-review pr 177578 run on x86_64-linux 1

23 packages built:
  • gdtoolkit
  • gdtoolkit.dist
  • godot3
  • godot3-debug-server
  • godot3-export-templates
  • godot3-headless
  • godot3-headless.dev
  • godot3-headless.man
  • godot3-mono
  • godot3-mono-debug-server
  • godot3-mono-export-templates
  • godot3-mono-headless
  • godot3-mono-headless.dev
  • godot3-mono-headless.man
  • godot3-mono-server
  • godot3-mono.dev
  • godot3-mono.man
  • godot3-server
  • godot3.dev
  • godot3.man
  • lorien
  • oh-my-git
  • pixelorama

lilyinstarlight
lilyinstarlight reviewed Jul 30, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Okay, I do have one more set of review items, but I'll leave them to your discretion tbh. If you do not want to fix the aarch64 build, just mark it as meta.broken = stdenv.isAarch64; or something. Otherwise I did just test what I suggested as a change and it works for aarch64-linux on the community builder

Thank you again for this work!

lilyinstarlight
lilyinstarlight reviewed Jul 30, 2023
View reviewed changes
Rotaerk added 2 commits July 30, 2023 18:46
@Rotaerk
godot3: refactor and rename from godot
e138e65 
The godot base derivation was implemented by passing a recursive
attrset to mkDerivation. This is problematic because recursive
references to attributes that are later overridden don't notice the
override. Switched to the approach of giving mkDerivation a lambda
that receives a self argument, which allows recursive references to
the final value after overrides are applied.

The related derivations (for export templates, headless, and server
builds of godot) duplicated content from the base derivation. This
refactor eliminated that by using overridable attributes to
parameterize the scripts.

The main motivation for this refactor was to help me add derivations
for mono builds of godot.

Renamed to godot3 to distinguish from version 4, which is a complete
rewrite and effectively a different tool altogether.
@Rotaerk
godot3: add mono builds
e555a7b
@Rotaerk
Copy link
Contributor Author

Rotaerk commented Jul 30, 2023

Okay, the changes have been made. Here is what I'd suggest for testing this on aarch64:

  1. Get godot3 building.
  2. Clone https://github.com/godotengine/godot-demo-projects, and checkout branch 3.5.
  3. Run godot3 and confirm that it starts without any library load errors.
  4. Within godot, you should be presented with a "Local Projects" screen. Click the Import button on the right, and select the godot-demo-projects/2d/dodge_the_creeps/project.godot file. When that project is open, hit F5 (or click the Play button in the top right) to run the game. This particular game has a Start button that you can click to start a round. Confirm that it's playable and that there's music playing (the music doesn't start until the round starts).
  5. Build godot3-mono. Repeat steps 3 and 4 for it, but using godot-demo-projects/mono/dodge_the_creeps/project.godot instead.
  6. Build and run pixelorama, confirming that it starts fine without library load errors.
  7. Repeat this for lorien and oh-my-git. The latter should have some music playing in the main menu, although it's somewhat faint.

lilyinstarlight
lilyinstarlight approved these changes Aug 16, 2023
View reviewed changes
Copy link
Member

@lilyinstarlight lilyinstarlight left a comment

Choose a reason for hiding this comment

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

Okay I've given this one last look over and it's an impressive amount of work!

I didn't do much aarch64 testing (since I haven't had the time to set up NixOS on one of my Raspberry Pis or something, which I'm not even sure Godot will support anyway), but most of the applications don't even include aarch64-linux in meta.platforms, so I doubt it gets much use on that platform and given it actually builds now, this PR is likely an improvement regardless there

I have no further comments from a perspective of actual problems (any comments I would have would mostly be preferential or stylistic) and I don't see any unresolved review comments, so I'll say I think this is ready!

I'll give it just another couple days for anyone else to bring up potential issues or correct me if not all review comments are addressed, but if there's no dissent then I'll go ahead and merge this

Thank you for sticking with this @Rotaerk and doing all of this work, which I'm sure people appreciate ❤️

@lilyinstarlight lilyinstarlight merged commit 3effe8e into NixOS:master Aug 18, 2023
@Rotaerk
Copy link
Contributor Author

Rotaerk commented Aug 18, 2023

Awesome, thanks @lilyinstarlight and @Janik-Haag for helping me with finalizing this! And thanks everyone else who has provided feedback throughout the life of this PR. It's much better than it originally was.

@kirillrdy kirillrdy mentioned this pull request Mar 19, 2024
Merged
13 tasks
@tomodachi94 tomodachi94 mentioned this pull request May 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mindavi Mindavi Mindavi left review comments

@winterqt winterqt winterqt left review comments

@jojosch jojosch Awaiting requested review from jojosch

@lihop lihop Awaiting requested review from lihop

@SuperSandro2000 SuperSandro2000 Awaiting requested review from SuperSandro2000

@GovanifY GovanifY Awaiting requested review from GovanifY

@90-008 90-008 Awaiting requested review from 90-008

@shiryel shiryel Awaiting requested review from shiryel

@hesiod hesiod Awaiting requested review from hesiod

@hqurve hqurve Awaiting requested review from hqurve

@felschr felschr Awaiting requested review from felschr

@Twey Twey Awaiting requested review from Twey

+4 more reviewers

@leiserfg leiserfg leiserfg left review comments

@lilyinstarlight lilyinstarlight lilyinstarlight approved these changes

@Janik-Haag Janik-Haag Janik-Haag approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

8.has: clean-up This PR removes packages or removes other cruft 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in any of the changed packages.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.