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
texlab: 1.10.0 -> 2.0.0 #85700
texlab: 1.10.0 -> 2.0.0 #85700
Conversation
@GrahamcOfBorg build texlab texlab.passthru.tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Builds/runs on x86_64-darwin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I too favor this PR vs #85640 . Seems to run fine after testing completion in a small file.
(On x86_64 Linux). |
Shouldn't we be encoding runtime dependencies in the derivation? Without a wrapper, texlab will depend on the TeX distribution installed in the user's environment, which is impure. |
I think latex counts as a compiler, where the user is expected to provide build dependencies. |
@MetaDark I agree with @veprbl and @symphorien on this - Texlab is just a language server, it's not like we ship software that is built with it. Users only expect it to give them completion to their code - nothing more fancy then that. |
@symphorien A wrapper doesn't prevent a user from specifying the build dependency, it makes it more explicit, while providing a sensible default. It can be overridden as such: { config, pkgs, ... }:
{
environment.systemPackages = with pkgs; [
(texlab.override { tex = tectonic; })
];
} This is somewhat like stdenv, which provides gcc by default, but can be configured to use another compiler. Being more explicit about the the specific TeX distribution that texlab depends on also means that if you have multiple versions installed, it will use the one that's specified, rather than the first one that texlab finds. |
Overriding would make sense only if texlab wanted a configuration file with absolute paths to to latexmk and so on before compilation. Providing latex by PATH is
|
@MetaDark I see how that wrapping can be overridden with an overlay. Try to answer this question: What would a Usually, wrapping is performed in order to make sure a package will be able to perform it's basic functions out of the box (e.g gopass). If there's an optional feature available only when another input is added to a wrapper, then adding a wrapper to Nixpkgs is a good idea (e.g zathura). In this case, the wrapper doesn't bring any improvement, for both tectonic and texlive users. Since the necessary binaries are usually available anyway in one's system - as it'd be sensible to assume if a user uses a texlive distro supported by texlab. |
@doronbehar If a user wants to use tectonic instead of texlive, it will replace texlive. It won't be included if the user doesn't want it. |
@MetaDark you are not answering the question:
Do you honestly think that's a sensible default behavior? By "default" I mean the behavior a user'd get if they use no overlays. |
And by no overlays I also refer to not any of |
@doronbehar Yes, I do think it's a sensible default behviour, texlab is clear about that the fact that it requires a TeX distribution when you first start it without one installed (even though it doesn't completely prevent usage), so I think we should provide one by default. The same thing is done for:
and if the user really wanted, they could use |
@MetaDark your examples:
Are packages that actually produce documents. Moreover, they are used as dependencies within the Nixpkgs collection so it makes sense to make them wrapped in a somewhat configurable manner. Contrary to these examples, texlab is just a language server, it won't crash or something horrible like that if it won't find a supported Tex distribution in it's PATH. Moreover: your wrapper fixes a sharp edge case: A case where a user is using Texlab, and they don't have a Tex distro installed at all. I can't imagine why'd someone have such a setup. If we'll go your way, tectonic users (which I think is a large part of Texlab's users) will pay a price of inconvenience at their next update: They'll either notice the closure size increase and will be too lazy to investigate it. Or, they will investigate it and find this discussion, where most of the involved users where against the wrapper, and they'll conclude to either debate this in another PR (where they'd suggest to change the default) or to use an override. My opinion, is that the collective inconvenience price is higher then the outcome. It's OK to have opinions @MetaDark , that's why we are using GNU/Linux and not a closed source OS :). But, we need to get to a decision at some point. Hence I'd kindly ask @veprbl which has merge permissions to overrule this. |
We are definitely merging the version bump, unless someone has objections. Addition of the wrapper appears to be controversial. I think it's unlikely that it will be merged in its current form. Perhaps, we should ask a question: would it be acceptable to merge a PATH wrapper with some compromises? What would be the changes needed after it becomes acceptable? |
@veprbl Sounds good to me, in general I think it's better to explicitly encode runtime dependencies, but for this specific case I've been convinced that it's OK to rely on the system installed dependency. |
The only compromise I can imagine is to put nothing by default in the PATH of the wrapper. Because both texlive users and tectonic users won't want any of the other tex distro downloaded to their closure if they don't use them. And, nothing makes either distro more "mainline" to make it the "default" we should almost enforce on users. Even then, I can hardly imagine anyone using this wrapping feature via an override, as most users of texlab already have a tex distro installed and texlab is smart enough to detect the executables already available. Hence that wouldn't bring users any benefit, especially considering the small cost of having a new derivation and a new |
Thank you @MetaDark for agreeing :). You can always use a wrapping override yourself BTW. |
Great! Thanks everyone! |
This issue has been mentioned on NixOS Discourse. There might be relevant details there: |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
https://github.com/latex-lsp/texlab/releases/tag/v2.0.0
Tested briefly on a few files.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)