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

texlab: 1.10.0 -> 2.0.0 #85700

Merged
merged 1 commit into from Apr 22, 2020
Merged

texlab: 1.10.0 -> 2.0.0 #85700

merged 1 commit into from Apr 22, 2020

Conversation

symphorien
Copy link
Member

https://github.com/latex-lsp/texlab/releases/tag/v2.0.0

Tested briefly on a few files.

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.

@veprbl
Copy link
Member

veprbl commented Apr 21, 2020

@GrahamcOfBorg build texlab texlab.passthru.tests

Copy link
Member

@veprbl veprbl left a 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

Copy link
Contributor

@doronbehar doronbehar left a 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.

@doronbehar
Copy link
Contributor

(On x86_64 Linux).

@kira-bruneau
Copy link
Contributor

kira-bruneau commented Apr 21, 2020

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.

@symphorien
Copy link
Member Author

I think latex counts as a compiler, where the user is expected to provide build dependencies.

@doronbehar
Copy link
Contributor

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

@kira-bruneau
Copy link
Contributor

kira-bruneau commented Apr 22, 2020

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

@symphorien
Copy link
Member Author

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

  • simpler in the nix code
  • respects nix-shell with custom texlive
  • simpler for the user in the shell because more intuitive
    Also note that you are providing optional dependencies here. An unwrapped texlab is perfectly functional as automated build is off by default.

@doronbehar
Copy link
Contributor

@MetaDark I see how that wrapping can be overridden with an overlay. Try to answer this question: What would a tectonic user think, on his next update, when he'll find out all of a sudden that one of his packages needs all of texlive.combined? Do you honestly think that's a sensible default behavior? All this for the sake of "purity" for texlab's runtime environment? It's only a language server - It's behavior shouldn't be considered that sensitive in comparison to compilers.

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.

@kira-bruneau
Copy link
Contributor

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

@doronbehar
Copy link
Contributor

@MetaDark you are not answering the question:

What would a tectonic user think, on his next update, when he'll find out all of a sudden that one of his packages needs all of texlive.combined?

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.

@doronbehar
Copy link
Contributor

And by no overlays I also refer to not any of texlab.overrides { this = that; }.

@kira-bruneau
Copy link
Contributor

kira-bruneau commented Apr 22, 2020

@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:

  • asciidoc
  • ipe
  • pidgin-latex
  • sphinxcontrib-tikz
  • texmacs

and if the user really wanted, they could use texlab-unwrapped.

@doronbehar
Copy link
Contributor

@MetaDark your examples:

  • asciidoc
  • ipe
  • pidgin-latex
  • sphinxcontrib-tikz
  • texmacs

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.

@veprbl
Copy link
Member

veprbl commented Apr 22, 2020

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?

@kira-bruneau
Copy link
Contributor

kira-bruneau commented Apr 22, 2020

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

@doronbehar
Copy link
Contributor

would it be acceptable to merge a PATH wrapper with some compromises?

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

@doronbehar
Copy link
Contributor

Thank you @MetaDark for agreeing :). You can always use a wrapping override yourself BTW.

@veprbl
Copy link
Member

veprbl commented Apr 22, 2020

Great! Thanks everyone!

@veprbl veprbl merged commit 9aa4a4c into NixOS:master Apr 22, 2020
@veprbl veprbl mentioned this pull request Apr 22, 2020
10 tasks
@nixos-discourse
Copy link

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/should-we-support-encoding-runtime-dependencies-without-requiring-a-specific-implementation/21031/6

@symphorien symphorien deleted the update-texlab branch August 16, 2022 20:56
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/should-we-support-encoding-runtime-dependencies-without-requiring-a-specific-implementation/21031/10

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

5 participants