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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

minecraft: mark broken #299645

Merged
merged 1 commit into from
May 8, 2024
Merged

Conversation

tomberek
Copy link
Contributor

@tomberek tomberek commented Mar 28, 2024

Description of changes

The minecraft launcher will fail for new versions. This results in a hard-to-understand error. Either we mark the package as broken (yes, it does work when launching old versions) or provide a warning.

Fixes: #179323

Things done

Added a warning..... asking for feedback if marking broken = true; would just be better.

  • 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.05 Release Notes (or backporting 23.05 and 23.11 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.

@TLATER
Copy link
Contributor

TLATER commented Mar 31, 2024

I think the warning is better than marking it broken, if only because this message shows the alternatives.

@sudoforge
Copy link
Contributor

sudoforge commented Mar 31, 2024

I think the warning is better than marking it broken, if only because this message shows the alternatives.

i believe that the message should be shown on stderr prior to true being evaluated:

trace e1 e2

Evaluate e1 and print its abstract syntax representation on standard error. Then return e2. This function is useful for debugging.

https://nixos.org/manual/nix/stable/language/builtins#builtins-trace


that said, it looks like emitting something to stderr causes the ofborg-eval step to fail. perhaps it's not possible to use builtins.trace here after all.

@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-in-distress/3604/93

@doronbehar
Copy link
Contributor

I have a bottom line question (haven't read all prior discussions): Is the derivation actually usable? For anyone?

@sudoforge
Copy link
Contributor

I have a bottom line question (haven't read all prior discussions): Is the derivation actually usable? For anyone?

no, it is broken.

@doronbehar
Copy link
Contributor

Then why not simply broken = true?

@sudoforge
Copy link
Contributor

sudoforge commented May 5, 2024

Then why not simply broken = true?

that's described by the author of the change, in one of the four comments you decided to skip:

#299645 (comment)

@tomberek
Copy link
Contributor Author

tomberek commented May 5, 2024

I'm willing to go in either direction. Just let me know.

@doronbehar
Copy link
Contributor

This is a broad issue in Nixpkgs - that we lack a robust way to describe why packages are broken, besides writing a comment above the broken = true; line. I suggest to not insist on inventing a solution to this issue here, and skip to the common approach of simply broken = true; along with a comment.

@TLATER
Copy link
Contributor

TLATER commented May 7, 2024

I have a bottom line question (haven't read all prior discussions): Is the derivation actually usable? For anyone?

no, it is broken.

It actually is usable, so long as you don't try to play relatively recent minecraft versions with it. Older versions still work because they don't bundle natively compiled code.

This is why I was hesitant to mark it broken in the first place. But other than that, broken + comment is probably fine for now, and we should invent a better solution for these things.

@tomberek tomberek force-pushed the tomberek.minecraft_broken branch from 8d451d8 to 8b34e45 Compare May 7, 2024 17:58
@tomberek
Copy link
Contributor Author

tomberek commented May 7, 2024

updated to accommodate the consensus that marking "broken" is better

@tomberek tomberek changed the title minecraft: warn of failure minecraft: mark broken May 7, 2024
@doronbehar doronbehar merged commit 5fab32d into NixOS:master May 8, 2024
26 of 27 checks passed
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.

Minecraft client no longer works on NixOS
5 participants