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.

Sign up for GitHub

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

Jump to bottom

etlegacy: fix and refactor #250249

Merged
merged 11 commits into from Aug 21, 2023
Merged

etlegacy: fix and refactor #250249

merged 11 commits into from Aug 21, 2023

Conversation

drupol
Copy link
Contributor

@drupol drupol commented Aug 19, 2023 •
edited

Hello,

I'm unable to run etlegacy on my machine, I get the following error log:

❯ etl.x86_64
tty]*** buffer overflow detected ***: terminated
*** buffer overflow detected ***: terminated
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
*** buffer overflow detected ***: terminated
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
*** buffer overflow detected ***: terminated
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
*** buffer overflow detected ***: terminated
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
...
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
*** buffer overflow detected ***: terminated
DOUBLE SIGNAL FAULT: Received signal 6, exiting...
*** buffer overflow detected ***: terminated
fish: Job 1, 'etl.x86_64' terminated by signal SIGSEGV (Address boundary error)
~ ✘

This PR:

  • Added the etl and etlded binaries
  • Disable fortify to have fully working binaries and fix the aforementioned issue
  • Remove unused runCommand argument
  • Add myself as maintainer
  • Reduce amount of local vars
  • Refactor fetchAsset function
  • Build server and client
  • Update cmakeFlags and change default install directories

Ping @ashleyghooper

Description of changes

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/)
  • 23.11 Release Notes (or backporting 23.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.

@drupol drupol mentioned this pull request Aug 19, 2023
Merged
12 tasks
@drupol drupol marked this pull request as ready for review August 19, 2023 20:44
@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/1016/69

@ofborg ofborg bot requested a review from ashleyghooper August 19, 2023 20:55
@bjornfor
Copy link
Contributor

etlegacy: remove mainProgram attribute

It's not removing it though, just refactoring.

ashleyghooper
ashleyghooper suggested changes Aug 20, 2023
View reviewed changes
Copy link
Contributor

@ashleyghooper ashleyghooper left a comment

Choose a reason for hiding this comment

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

Hi @drupol
thanks for your suggested changes. Many of them look okay, and adding the dedicated server is a good move, but I'd like to discuss a few of them. See comments in the review.

Also, I'm keen to hear your thoughts on how we support for other platforms than Linux x86_64. Especially Linux i686 and Darwin.

pkgs/games/etlegacy/default.nix
# This indicates the build was by a CI pipeline and prevents the resource
# files from being flagged as 'dirty' due to potentially being custom built.
export CI="true"
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand the motivation to optimise, but I would prefer to retain the preBuild script as it was requested we add those variables by one of the ETLegacy developers who reviewed an earlier build.

Without CI=true the game client shows the version as 2.81-dirty instead of 2.81.1 which may cause confusion. Compare with other packages, including Flatpak, where the version shows correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, going to revert my change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for that. Previously I think there was a suggestion we could talk to upstream about getting an upstream change to make this unnecessary. I guess it's there to make it clear to people who compile themselves and might have modified things that they're running a version that could potentially be unstable when playing online.

pkgs/games/etlegacy/default.nix Outdated
makeWrapper $out/lib/etlegacy/etl.* $out/bin/etl
makeWrapper $out/lib/etlegacy/etlded.* $out/bin/etlded

rm -rf $out/share/applications/com.etlegacy.ETLegacy.x86_64.desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

As an ET fan, you're probably aware that most ET mods don't run on the 64-bit version. Ergo, many people, including myself, want to run the 32-bit version. The change to postInstall would result in duplicate desktop files for anybody running versions other than the x86_64 version.

Also, just rm (without -rf would be enough here, since we're just removing a single file). :)

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're right, I've skipped the x86_64 in the filename, going to make an adjustment.

pkgs/games/etlegacy/default.nix
mainProgram =
if stdenv.hostPlatform.system == "i686-linux"
then "etl.i386"
else "etl.x86_64";
Copy link
Contributor

Choose a reason for hiding this comment

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

I liked that the above enabled me to have both 64-bit and 32-bit (via pkgsi686Linux) versions installed without a collision on the executable - I can execute either executable to launch that version (and also can choose either .desktop file). I admit that's probably not a particularly strong need though and of course an override could restore distinct executable names if versions for multiple architectures were installed.

However, it also meant we could just use upstream's .desktop file instead of deleting theirs and taking responsibility for a new one as part of this build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough, going to leave the .<arch> in the next iteration and use their desktop file.

pkgs/games/etlegacy/default.nix
if [ "$1" = "describe" ]; then
echo "${version}"
fi
'';
Copy link
Contributor

Choose a reason for hiding this comment

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

The fakeGit seemed to be necessary to build previously, as the build process runs git describe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Going to restore this as well.

@ashleyghooper ashleyghooper self-assigned this Aug 20, 2023
@drupol drupol marked this pull request as draft August 20, 2023 16:03
@drupol
Copy link
Contributor Author

drupol commented Aug 20, 2023

Thanks for the detailed review, I'm going to start working on them in one hour.

@drupol drupol marked this pull request as ready for review August 20, 2023 17:18
@drupol
Copy link
Contributor Author

drupol commented Aug 20, 2023 •
edited

I think I've addressed everything, could you please check again?

I've installed temporarily with nix profile install and everything is OK!!!
It's been at least 10 years that I haven't played, it's so great to be able to play again :)

image

@ofborg ofborg bot requested a review from ashleyghooper August 20, 2023 17:41
@drupol drupol merged commit b80ada5 into master Aug 21, 2023
21 checks passed
@drupol
Copy link
Contributor Author

drupol commented Aug 21, 2023

Youhou ! thanks !

@drupol drupol deleted the etlegacy/fix-and-refactor branch August 21, 2023 10:42
@ashleyghooper ashleyghooper mentioned this pull request Apr 7, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ashleyghooper ashleyghooper ashleyghooper approved these changes

Assignees

@ashleyghooper ashleyghooper

Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@drupol @nixos-discourse @bjornfor @ashleyghooper