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

blockbench: 4.8.1 -> 4.9.4, build from source, rename package #279795

Merged
merged 1 commit into from
Mar 16, 2024
Merged

blockbench: 4.8.1 -> 4.9.4, build from source, rename package #279795

merged 1 commit into from
Mar 16, 2024

Conversation

TomaSajt
Copy link
Contributor

@TomaSajt TomaSajt commented Jan 9, 2024 •
edited
Loading

Description of changes

Closes #279495
Supersedes #264654 (has the same fix)
Related tracking issue: #295770

Other than changing the version, I also changed the package to use buildNpmPackage.

I moved the package over to pkgs/by-name under the new name of blockbench
I also added an alias so as to not break existing configs with the rename.

According to upstream, this should use electron_26. However, one of my machines always crashes when running electron_26, so I used electron_28 which should not be EOL for a while. It seems to run okay even with a version mismatch.

Other changes:

  • remove with lib; from meta
  • set meta.changelog
  • update meta.description
  • set meta.mainProgram
  • set meta.broken to signal broken darwin builds
  • manually generate icons (using the --dir flag on electron-builder unfortunately doesn't generate the icons)
  • add myself to meta.maintainers

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

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jan 9, 2024
@ofborg ofborg bot requested a review from ckiee January 9, 2024 11:27
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Jan 9, 2024
@ckiee
Copy link
Member

ckiee commented Jan 9, 2024

this looks wonderful at a first glance! I think you should go ahead with that rename, making sure to preserve backwards compat using pkgs/top-level/aliases.nix. maybe try by-name out if you're in the mood, blockbench is a good fit.

@TomaSajt TomaSajt changed the title blockbench-electron: 4.8.1 -> 4.9.3, build from source blockbench: 4.8.1 -> 4.9.3, build from source, rename package Jan 9, 2024
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Jan 9, 2024
@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 9, 2024

Huh, seems like all current PR workflows are getting internal server errored

@TomaSajt
Copy link
Contributor Author

TomaSajt commented Jan 9, 2024

@ckiee previously there was a line which added cc to the library path, which I removed:

--prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ stdenv.cc.cc ]}"

Why was this needed originally? Was it just something needed for pre-packaged electron apps?

@cole-h cole-h removed the ofborg-internal-error Ofborg encountered an error label Jan 9, 2024
@ckiee
Copy link
Member

ckiee commented Jan 9, 2024

@ckiee previously there was a line which added cc to the library path, which I removed:

--prefix LD_LIBRARY_PATH : "${lib.makeLibraryPath [ stdenv.cc.cc ]}"

Why was this needed originally? Was it just something needed for pre-packaged electron apps?

yes, i think they just shipped some weird binary, i'm not too sure - probably a libstdc++ thing

@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Jan 21, 2024
dotlambda
dotlambda reviewed Jan 31, 2024
View reviewed changes
pkgs/by-name/bl/blockbench/package.nix Outdated Show resolved Hide resolved
pkgs/by-name/bl/blockbench/package.nix Outdated Show resolved Hide resolved
@TomaSajt TomaSajt force-pushed the blockbench branch 5 times, most recently from 42662c8 to f9e1310 Compare February 1, 2024 16:10
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 and removed 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin labels Feb 1, 2024
@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 8, 2024
@ckiee ckiee mentioned this pull request Mar 16, 2024
Closed
13 tasks
@TomaSajt TomaSajt changed the title blockbench: 4.8.1 -> 4.9.3, build from source, rename package blockbench: 4.8.1 -> 4.9.4, build from source, rename package Mar 16, 2024
@ckiee
Copy link
Member

ckiee commented Mar 16, 2024

Oh sorry about that other PR I lost track of this one. I'll take a look.

ckiee
ckiee reviewed Mar 16, 2024
View reviewed changes
pkgs/by-name/bl/blockbench/package.nix Outdated
desktopItems = [
(makeDesktopItem {
name = "blockbench";
exec = "blockbench --no-sandbox %U";
Copy link
Member

@ckiee ckiee Mar 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't like the --no-sandbox, we probably don't need it.
(Pushed a fix)

ckiee
ckiee approved these changes Mar 16, 2024
View reviewed changes
Copy link
Member

@ckiee ckiee left a comment

Choose a reason for hiding this comment

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

nice work! (:

@ckiee ckiee merged commit a60c759 into NixOS:master Mar 16, 2024
5 of 6 checks passed
@ofborg ofborg bot requested a review from ckiee March 16, 2024 04:12
@TomaSajt TomaSajt mentioned this pull request Mar 16, 2024
Closed
13 tasks
@yu-re-ka yu-re-ka mentioned this pull request Mar 16, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotlambda dotlambda dotlambda left review comments

@h7x4 h7x4 h7x4 approved these changes

@ckiee ckiee Awaiting requested review from ckiee

Assignees
No one assigned
Labels
8.has: clean-up 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 10.rebuild-darwin: 1 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update request: blockbench-electron 4.8.1 → 4.9.3
7 participants
@TomaSajt @ckiee @dotlambda @h7x4 @delroth @wegank @cole-h