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 #85640

Closed
wants to merge 1 commit into from
Closed

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Apr 20, 2020

Also added a configurable wrapper that puts a TeX Distribution into PATH.
Defaults to texlive.combined.scheme-small.

Without this wrapper, the following message is displayed on startup:

Your TeX distribution could not be detected. Please make sure that your distribution is in your PATH.
Motivation for this change

Upgrade to latest version of texlab: https://github.com/latex-lsp/texlab/releases/tag/v2.0.0

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.

@doronbehar

Also added a configurable wrapper that puts a TeX Distribution into PATH.
Defaults to texlive.combined.scheme-small.

Without this wrapper, the following message is displayed on startup:
Your TeX distribution could not be detected. Please make sure that your distribution is in your PATH.
@veprbl
Copy link
Member

veprbl commented Apr 21, 2020

Why do you need the wrapper? texlive and texlab seem to be perfectly composable right now – they just need to be installed together in nix-env/configure.nix/nix-shell.

@kira-bruneau
Copy link
Contributor Author

@veprbl The wrapper was added to ensure purity, so that the language server isn't dependent on the environment, and fully functionally without having to install additional packages. Why is this a problem?

@veprbl
Copy link
Member

veprbl commented Apr 22, 2020

@MetaDark You say "purity", but what you describe is actually "isolation". Latter, to some degree, can be achieved in different many ways depending on what task you have. It's really up to user to decide what kind of isolation they want to have. The problem is, I think, that putting texlive.combined.scheme-small for default setting would not satisfy any of the users, and may lead to some surprises for those who are not expecting this wrapper.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Apr 22, 2020

@veprbl What I describe is isolation, but also purity. Relying on whatever is in PATH to resolve dependencies is relying on global mutable state, which is impure.

The wrapper also doesn't prevent users from deciding what kind of "isolation" they want to have, it makes the decision more explicit:

{ config, pkgs, ... }:

{
  environment.systemPackages = with pkgs; [
    (texlab.override { tex = tectonic; })
  ];
}

If you had multiple TeX distributions installed, then texlab would just select whatever one it finds first, which may not be the one you want. If anything, this should lead to fewer suprises.

The same kind of thing is done for TeX distributions on:

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

Some projects do the patching in the patch / fixup phases, but I created a separate derivation so texlab doesn't need to be rebuilt to change the TeX distribution.

@veprbl
Copy link
Member

veprbl commented Apr 22, 2020

Closing in favour of #85700

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

2 participants