conda-forge / quarto-feedstock

A conda-smithy repository for quarto.
BSD 3-Clause "New" or "Revised" License
4 stars 9 forks source link

Remove patch to install TinyTeX relative to conda environment #24

Closed mfisher87 closed 1 year ago

mfisher87 commented 1 year ago

Resolves #23

Previously, the entire conda environment was being overwritten by the command quarto install tinytex. Now, TinyTex will be installed to <env>/share/TinyTex.

This required an additional hack in the code for detecting TinyTeX:

When hasTexLive was called, it was detecting an installation, but quarto was still looking for (and failing to find) tlmgr at the patched TinyTeX install location (previously: <env>/bin/; now: <env>/share/TinyTex/bin/). So I simplified that function to only look for the Quarto-installed version of TinyTeX and skip the search for TeXLive.

This is a workaround; long-term, it would be great to improve the logic for utilizing tlmgr if it's already on the user's PATH.

Checklist

conda-forge-webservices[bot] commented 1 year ago

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

mfisher87 commented 1 year ago

@conda-forge-admin, please rerender

mfisher87 commented 1 year ago

FYI: I pushed up a v1.3.427 branch to also apply this patch to that version. I'm not sure how to set up a review workflow for maintaining several versions.

cderv commented 1 year ago

Have you identified why this patch is needed ? What would the search for tlmgr fails in the context of Conda environment ? Do you think it is specific to conda or there should be a solution to quarto-cli directly ?

mfisher87 commented 1 year ago

Hey Christophe, thanks for taking a look. I didn't realize how long this message would turn out to be until after I posted, so thanks for your patience :)

This patch long predates my maintership on this repository, so I'm lacking the full context needed. I only had limited time to dig around and start to understand the Quarto code. I'm interested to learn more and start contributing to the Quarto code base some time, but currently fairly time-limited! Maybe @msarahan will have some more thoughts! I'll do my best:

Have you identified why this patch is needed ?

This patch was initialized when this feedstock was created, but lacked documented context. I'm adding documentation to each patch file as I learn more. To me, it seems clear that the original goal was to enable environment-specific installs of TinyTeX. Maybe that's not the correct path forward; maybe all Quarto installations should share one TinyTex installation instead of having independent ones. So maybe the patch isn't needed at all?

At the time that this feedstock was initialized, for Linux systems tinyTexInstallDir would return the conda environment directory, but maybe that didn't always manifest with this env-destroying bug: if Quarto's code was different at that time, perhaps it used to install TinyTeX into a subdir of that directory. Today, on TinyTex install, the contents of tinyTexInstallDir are wholesale replaced with TinyTex, so we can't continue patching that path to be the same as the conda env. I'm not sure if that was always true, and if not, when it changed.

Why would the search for tlmgr fails in the context of Conda environment ?

That's a really good question. I wasn't able to fully figure that out. Here's what it looks like for me (I have a system install of TinyTeX) to try to use TinyTex with a version of the Quarto conda package prior to this PR:

$ mamba create -n test-quarto-tinytex quarto=1.3.433
# ... build number 0 is installed ...

$ conda activate test-quarto-tinytex
$ quarto tools list
[✓] Inspecting tools

Tool         Status                    Installed     Latest     
chromium     Not installed             ---           869685     
tinytex      External Installation     ---           v2023.07.08

It would be nice if the "External installation" status provided more context. I do have an external installation, so things look OK at this point, but they're not. Quarto (is it because of our patches?) attempts to run tlmgr from the place it would have been installed if Quarto had installed it:

$ quarto preview
ERROR: Error executing '/path/to/miniconda3/envs/test-quarto-tinytex/bin/tlmgr': No such file or directory (os error 2)

An attempt to resolve (note that in the log messages, Quarto sees the current version of TinyTeX as undefined) yields a confusing experience. If I have an external installation, I wouldn't be expect to be "upgrading" it by using Quarto to install TinyTex, I'd expect to be adding an "internal installation" that's separate from my "external installation".

$ ls -1 /path/to/miniconda3/envs/test-quarto-tinytex/
bin/
conda-meta/
etc/
lib/
share/

$ quarto install tinytex
 ? tinytex is already installed. Do you want to update to v2023.07.08? (Y/n) › Y
Updating TinyTeX from undefined to v2023.07.08
[✓] Downloading TinyTex v2023.07.08
Removing undefined
[✓] Removing directory
Installing v2023.07.08
[✓] Unzipping TinyTeX-v2023.07.08.tar.gz
[✓] Moving files
Finishing update
[✓] Verifying tlgpg support
[✓] Default Repository: https://mirrors.rit.edu/CTAN/systems/texlive/tlnet/
Update successful

$ ls -1 /path/to/miniconda3/envs/test-quarto-tinytex/
bin/
texmf-config/
texmf-dist/
texmf-local/
texmf-var/
tlpkg/
LICENSE.CTAN
LICENSE.TL
release-texlive.txt
texmf.cnf
texmfcnf.lua
version

Do you think it is specific to conda or there should be a solution to quarto-cli directly ?

I think it depends on your goals! Based on the messaging from quarto tools list, it looks like Quarto aspires to be capable of using a pre-existing "external installation" of TinyTex and managing a separate "internal installation" if necessary. As far as I can tell, the current code for finding and using TinyTex installations relies on both external and internal installations existing in the same location (IIRC ~/.TinyTex in Linux), so there is functionally no distinction between "external" and "internal" installations of TinyTex. It's more like a "pre-existing" installation than an "external" installation, and Quarto will just try and upgrade it in-place.

Having multiple isolated Quarto installs in multiple isolated locations each with or without their own "internal" TinyTex installs seems to have been the goal of our patch. Should quarto-cli start doing new things to support "internal" installs that don't conflict with "external" installs? OR should we remove that patch and let all installed copies of Quarto use the same TinyTeX install?

mfisher87 commented 1 year ago

Forgot to add: I think it's worthwhile pursuing the capability in quarto-cli to find TinyTeX installs in different places than ~/.TinyTeX. I think it would be wonderful to be able to install Quarto and TinyTex as separate conda dependencies and have them work together.

mfisher87 commented 1 year ago

We decided in #23 that this patch needs to go! :wastebasket:

quarto install tinytex should always install to ~/.TinyTeX. For a while, there will be no way to get an "isolated" TinyTeX install for Quarto in a specific conda environment. Eventually, we'll package TinyTeX for conda-forge (#26) and remove the need for users to do an extra step to install TinyTeX with Quarto plus enable multiple isolated versions of TinyTeX on the same system.

TODO:

mfisher87 commented 1 year ago

Patch is gone, tested, this is working great. Thanks @cderv for your collaboration on this :) Does this look good to you?