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

lmstudio: 0.2.22 -> 0.2.23 #310914

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

lmstudio: 0.2.22 -> 0.2.23 #310914

wants to merge 6 commits into from

Conversation

cig0
Copy link

@cig0 cig0 commented May 11, 2024

Description of changes

  • Upstream minor patch update update (0.2.22.x)
  • Updated download links

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.

@cig0
Copy link
Author

cig0 commented May 11, 2024

Hi everyone,
This is my first attempt to contribute to Nixpkgs.
While I thoroughly read the contribution guidelines, this PR will undoubtedly be lacking in many ways - I beg you all for patience.
I'm looking forward to learning from your observations!
Thanks.

@drupol
Copy link
Contributor

drupol commented May 11, 2024

Hi!

Thanks for your first contribution. Technically, it looks good.
However, is it really necessary to update to 0.2.22c? Can't we just wait for the next version?

@cig0
Copy link
Author

cig0 commented May 11, 2024

Hi!

Thanks for your first contribution. Technically, it looks good. However, is it really necessary to update to 0.2.22c? Can't we just wait for the next version?

Hi @drupol thanks for the quick reply -- and for maintaining this package!

I believe I failed to provide a better - more concise - context on why I opened this PR:

  • The current package is failing to install LM Studio (please see full log below)
  • I also noticed LM Studio devs also changed the download URL
$ nix-shell -p nix-info --run "nix-info -m"
 - system: `"x86_64-linux"`
 - host os: `Linux 6.8.9, NixOS, 24.05 (Uakari), 24.05pre623656.f1010e0469db`
 - multi-user?: `yes`
 - sandbox: `yes`
 - version: `nix-env (Nix) 2.18.2`
 - channels(root): `"nixos, nixos-hardware"`
 - nixpkgs: `/nix/var/nix/profiles/per-user/root/channels/nixos`

$ nix-shell -p lmstudio
these 5 derivations will be built:
  /nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv
  /nix/store/jv7k3czv5c7y47cvb2irj6wjhy81l240-lmstudio-0.2.22-extracted.drv
  /nix/store/dqdly8xpyikns094p4xic7lhhpblagh9-lmstudio-0.2.22-init.drv
  /nix/store/91dhzjwgh62k82fr72vpk60yjr2iygvr-lmstudio-0.2.22-bwrap.drv
  /nix/store/s35za31rmv18xswbmhxbraw5bbwrww1n-lmstudio-0.2.22.drv
building '/nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv'...

trying https://releases.lmstudio.ai/linux/0.2.22/beta/LM_Studio-0.2.22.AppImage
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  545M  100  545M    0     0  3968k      0  0:02:20  0:02:20 --:--:-- 4356k
error: hash mismatch in fixed-output derivation '/nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv':
         specified: sha256-hcV8wDhulFAxHDBDKicpEGovwcsn9RaIi/idUz+YzD8=
            got:    sha256-HoAVUU5Z5ueGMSybhT0OKV611v1yU5am0wy0GsCUmvk=
error: 1 dependencies of derivation '/nix/store/jv7k3czv5c7y47cvb2irj6wjhy81l240-lmstudio-0.2.22-extracted.drv' failed to build
error: 1 dependencies of derivation '/nix/store/s35za31rmv18xswbmhxbraw5bbwrww1n-lmstudio-0.2.22.drv' failed to build

@pluiedev
Copy link
Contributor

pluiedev commented May 12, 2024

$ nix-shell -p lmstudio
these 5 derivations will be built:
  /nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv
  /nix/store/jv7k3czv5c7y47cvb2irj6wjhy81l240-lmstudio-0.2.22-extracted.drv
  /nix/store/dqdly8xpyikns094p4xic7lhhpblagh9-lmstudio-0.2.22-init.drv
  /nix/store/91dhzjwgh62k82fr72vpk60yjr2iygvr-lmstudio-0.2.22-bwrap.drv
  /nix/store/s35za31rmv18xswbmhxbraw5bbwrww1n-lmstudio-0.2.22.drv
building '/nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv'...

trying https://releases.lmstudio.ai/linux/0.2.22/beta/LM_Studio-0.2.22.AppImage
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  545M  100  545M    0     0  3968k      0  0:02:20  0:02:20 --:--:-- 4356k

error: hash mismatch in fixed-output derivation '/nix/store/n394c0bgqizy2xgjxlmi7nainj7mvqqd-LM_Studio-0.2.22.AppImage.drv':
         specified: sha256-hcV8wDhulFAxHDBDKicpEGovwcsn9RaIi/idUz+YzD8=
            got:    sha256-HoAVUU5Z5ueGMSybhT0OKV611v1yU5am0wy0GsCUmvk=
error: 1 dependencies of derivation '/nix/store/jv7k3czv5c7y47cvb2irj6wjhy81l240-lmstudio-0.2.22-extracted.drv' failed to build
error: 1 dependencies of derivation '/nix/store/s35za31rmv18xswbmhxbraw5bbwrww1n-lmstudio-0.2.22.drv' failed to build

A hash mismatch? Now that is very odd. That means upstream is now delivering a different binary compared to before which is generally a pretty bad sign — if they can just change it willy-nilly, it potentially opens up downstream users for supply chain attacks. Bit hyperbolical, but it could happen.

@Luk45135
Copy link

Luk45135 commented May 12, 2024

A hash mismatch? Now that is very odd. That means upstream is now delivering a different binary compared to before which is generally a pretty bad sign — if they can just change it willy-nilly, it potentially opens up downstream users for supply chain attacks. Bit hyperbolical, but it could happen.

But isn't that always the case if it's not hosted on github. They can just change what binary is behind that link (and even if it is open source the maintainers would have to read every commit to verify that there is no malware which just isn't feasible.) Isn't that the reason we have this system to verify the hash before installing in the first place? Even then it's possible to change the binary without us knowing with a hash collision attack. To completely alleviate this issue we would have to host every binary on our own, which also isn't feasible without major server costs. I think this way of doing things is just the tradeoff we have landed on.
But i do think that it's odd that they changed the binary that's behind the link.

@pluiedev
Copy link
Contributor

But isn't that always the case if it's not hosted on github. They can just change what binary is behind that link (and even if it is open source the maintainers would have to read every commit to verify that there is no malware which just isn't feasible.) Isn't that the reason we have this system to verify the hash before installing in the first place? Even then it's possible to change the binary without us knowing with a hash collision attack. To completely alleviate this issue we would have to host every binary on our own, which also isn't feasible without major server costs. I think this way of doing things is just the tradeoff we have landed on.

But i do think that it's odd that they changed the binary that's behind the link.

Yeah, that is indeed the point, and we're not even allowed to host these binaries as they are unfree and not redistributable. However, it is quite worrying that they would just swap out the binary like that, which makes this source not reliable enough for our purposes. Stuff packaged in Nixpkgs has to build reliably and correctly, and that having a reliable source is a key part of that.

@eeedean
Copy link
Contributor

eeedean commented May 21, 2024

I would really like to keep the package available to all Nixpkgs users and would like to keep it up2date. However, I'm absolutely with @pluiedev. If LMStudio wont supply us with a reliable source, I'm not sure, if we should keep it in Nixpkgs altogether.

I would of course be absolutely fine with it being in Nixpkgs, as long as I or someone feels responsible for updating the package ASAP. But it's not really reliable at the time and if there was no maintainer available for a while, we would not just lag behind a couple versions, but would be stuck with a broken installation process (like right now), which isn't feasible.

Feel free to thumb up my issue at lmstudio or supply additional information and opinions to the topic.

@pluiedev
Copy link
Contributor

Looks like upstream has pledged to keep the download links stable for now. Let's just fix the link and the hash and get on with it now, then?

@eeedean
Copy link
Contributor

eeedean commented May 22, 2024

@cig0 will you update your remote branch to point to the most recent version?

@cig0
Copy link
Author

cig0 commented May 22, 2024

@eeedean @drupol Heads up: LM Studio devs updated the GNU/Linux filename package, adding -Ubuntu-20.04: 39adf42

At first, I thought about adding a variable to handle the new string, but I don't feel it'll change for a while since 20.04 is an LTS release.

I thought about letting you know before proceeding with the change —you guys are the package maintainers- but I preferred agility over bureaucracy here, as this is a minimal change.

Let me know what you think and if you prefer moving the OS bit to its own variable.

Thanks!

@drupol
Copy link
Contributor

drupol commented May 22, 2024

I have no strong opinion on this. I don't really use LMStudio anymore.

Can I ask you if you could remove my name from the maintainers list?

@cig0 cig0 changed the title lmstudio: 0.2.22 -> 0.2.22c lmstudio: 0.2.22 -> 0.2.23 May 23, 2024
Copy link
Contributor

@eeedean eeedean left a comment

Choose a reason for hiding this comment

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

Please adjust the commits by squashing them, where needed. Also take a look at the conventions for commit messages in CONTRIBUTING.md as well as README.md of pkgs.

The Commits should eventually look about like that:

  • maintainers: add cig0 or »your handle«
  • lmstudio: remove drupol from maintainers
  • lmstudio: add cig0 to maintainers
  • lmstudio: 0.2.22 -> 0.2.23

If you wouldn't want to be listed as maintainer, you can and should of course leave those commits out.

Thanks for your contribution!

Edit: I have no strong opinion about the -Ubuntu-20.04, too. Leave it, as you like 😺.

pkgs/by-name/lm/lmstudio/darwin.nix Outdated Show resolved Hide resolved
pkgs/by-name/lm/lmstudio/package.nix Outdated Show resolved Hide resolved
@cig0
Copy link
Author

cig0 commented May 23, 2024

@eeedean, I addressed the issues you pointed out (thanks!); let me know what you think.

@eeedean
Copy link
Contributor

eeedean commented May 24, 2024

It looks like you somehow pulled in commits, that don't belong to your branch.
image

Also the changes are incomplete and are not correctly distributed over the commits:

maintainers: add cig0 should only add your handle to maintainers/maintainer-list.nix. Be aware, that this list must be ordered alphabetically, or one of the checks will fail.
lmstudio: remove drupol from maintainers also does the actual update, which should go into lmstudio: 0.2.22 -> 0.2.23.

Best case would be to also add your own maintainer handle in a separate commit named like "lmstudio: adding cig0 to maintainers".

Also have a look at the failing check.

If you want to figure it out yourself first, you can have a look at the failing test. Otherwise, I got the reason written in the spoiler here 😸 You call
./darwin
without the revision variable
then callPackage ./darwin.nix { inherit pname version meta; }
but you should:
then callPackage ./darwin.nix { inherit pname version »revision« meta; }

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

8 participants