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

Sublime text 4 #119351

Closed
wants to merge 3 commits into from
Closed

Conversation

MasseGuillaume
Copy link
Contributor

Motivation for this change

resurect #93437

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

jtojnar and others added 2 commits April 13, 2021 13:17
* works with my 7 year old license so far
* i686 is no longer supported
* aarch64 support has been added
* adds harware accelerated (needs libGL.so.1)
* adds python 3.8 plugin host (needs libssl.so and libcrypto.so)
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
sha256 = archSha256;
};

dontStrip = true;
Copy link
Member

Choose a reason for hiding this comment

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

Why not strip?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's from this commit: d619f0e

Here they mention it's important for pre-built binary.
#15969 (comment)

I don't know why this is needed. I just copied the code from version 3 and adapted for 4.

Copy link
Member

Choose a reason for hiding this comment

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

Thats about dwarf fortress. If it doesn't crash sublime we should just strip it.

pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
pkgs/applications/editors/sublime/4/common.nix Outdated Show resolved Hide resolved
Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

It is not clear to me if SublimeHQ is okay with us packaging this at this time, which is why I did not merge the original PR.

}.${stdenv.hostPlatform.system};

libPath = lib.makeLibraryPath [ xorg.libX11 glib libglvnd openssl gtk3 cairo pango ];
redirects = [ "/usr/bin/pkexec=${pkexecPath}" ];
Copy link
Contributor

Choose a reason for hiding this comment

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

I kept the package updated in my pkgset:
https://github.com/jtojnar/nixfiles/blob/master/pkgs/sublime4/common.nix
It turned out that libredirect is more trouble than it is worth so I got rid of it:

jtojnar/nixfiles@38c413a

sublimehq/sublime_text#3502

@nh2
Copy link
Contributor

nh2 commented May 22, 2021

Sublime Text 4 is now released: https://www.sublimetext.com/blog/articles/sublime-text-4

@jtojnar
Copy link
Contributor

jtojnar commented May 22, 2021

Will update the PR ASAP.

mkdir -p "$out/share/applications"
substitute "''$${primaryBinary}/${primaryBinary}.desktop" "$out/share/applications/${primaryBinary}.desktop" --replace "/opt/${primaryBinary}/${primaryBinary}" "$out/bin/${primaryBinary}"
for directory in ''$${primaryBinary}/Icon/*; do
size=$(basename $directory)
Copy link
Contributor

Choose a reason for hiding this comment

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

$directory and $size could benefit from some quoting, given that they are are from the package and might contain spaces in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already iterate over * so that will not help, unless we use the ultra ugly find | read hack.

@jtojnar jtojnar mentioned this pull request May 22, 2021
9 tasks
@jtojnar
Copy link
Contributor

jtojnar commented May 22, 2021

Opened #124057

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.

None yet

4 participants