Skip to content

Deno.env review/cleanup: no set() past startup #12621

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/command/render/render-files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,7 @@ export async function renderExecute(
previewServer: context.options.previewServer,
handledLanguages: languages(),
project: context.project,
env: {},
};
// execute computations
setExecuteEnvironment(executeOptions);
Expand Down
12 changes: 7 additions & 5 deletions src/core/jupyter/jupyter-embed.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ import { ProjectContext } from "../../project/types.ts";
import { logProgress } from "../log.ts";
import * as ld from "../../../src/core/lodash.ts";
import { texSafeFilename } from "../tex.ts";
import { ExecuteOptions } from "../../execute/types.ts";

export interface JupyterNotebookAddress {
path: string;
Expand Down Expand Up @@ -415,7 +416,7 @@ export async function notebookMarkdown(

// Wrap any injected cells with a div that includes a back link to
// the notebook that originated the cells
const notebookMarkdown = (
const notebookMarkdownInner = (
nbAbsPath: string,
cells: JupyterCellOutput[],
title?: string,
Expand Down Expand Up @@ -454,7 +455,7 @@ export async function notebookMarkdown(
return cell;
}
});
return notebookMarkdown(nbAbsPath, theCells, notebookInfo.title);
return notebookMarkdownInner(nbAbsPath, theCells, notebookInfo.title);
} else if (nbAddress.indexes) {
// Filter and sort based upon cell indexes
const theCells = nbAddress.indexes.map((idx) => {
Expand All @@ -472,11 +473,11 @@ export async function notebookMarkdown(
return cell;
}
});
return notebookMarkdown(nbAbsPath, theCells, notebookInfo.title);
return notebookMarkdownInner(nbAbsPath, theCells, notebookInfo.title);
} else {
// Return all the cell outputs as there is no addtional
// specification of cells
const notebookMd = notebookMarkdown(
const notebookMd = notebookMarkdownInner(
nbAbsPath,
notebookInfo.outputs,
notebookInfo.title,
Expand Down Expand Up @@ -595,7 +596,8 @@ async function getCachedNotebookInfo(

// Get the markdown for the notebook
const format = context.format;
const executeOptions = {
const executeOptions: ExecuteOptions = {
env: {},
target: context.target,
resourceDir: resourcePath(),
tempDir: context.options.services.temp.createDir(),
Expand Down
12 changes: 6 additions & 6 deletions src/execute/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ export const setExecuteEnvironment: (options: ExecuteOptions) => void = (
options,
) => {
if (options.projectDir) {
Deno.env.set("QUARTO_PROJECT_ROOT", options.projectDir);
Deno.env.set("QUARTO_DOCUMENT_PATH", dirname(options.target.source));
Deno.env.set("QUARTO_DOCUMENT_FILE", basename(options.target.source));
options.env["QUARTO_PROJECT_ROOT"] = options.projectDir;
options.env["QUARTO_DOCUMENT_PATH"] = dirname(options.target.source);
options.env["QUARTO_DOCUMENT_FILE"] = basename(options.target.source);
} else {
// FIXME: This should not be passthrough anymore as singleFileProjectContext always set `options.projectDir`
// https://github.com/quarto-dev/quarto-cli/pull/8771
Expand All @@ -23,8 +23,8 @@ export const setExecuteEnvironment: (options: ExecuteOptions) => void = (
"No project directory or current working directory",
);
}
Deno.env.set("QUARTO_PROJECT_ROOT", options.cwd);
Deno.env.set("QUARTO_DOCUMENT_PATH", options.cwd);
Deno.env.set("QUARTO_DOCUMENT_FILE", basename(options.target.source));
options.env["QUARTO_PROJECT_ROOT"] = options.cwd;
options.env["QUARTO_DOCUMENT_PATH"] = options.cwd;
options.env["QUARTO_DOCUMENT_FILE"] = basename(options.target.source);
}
};
4 changes: 4 additions & 0 deletions src/execute/jupyter/jupyter-kernel.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ export async function executeKernelOneshot(
"execute",
{ ...options, debug },
options.kernelspec,
options.env,
);

if (!result.success) {
Expand Down Expand Up @@ -186,6 +187,7 @@ async function execJupyter(
command: string,
options: Record<string, unknown>,
kernelspec: JupyterKernelspec,
env: Record<string, string> = {},
): Promise<ProcessResult> {
try {
const cmd = await pythonExec(kernelspec);
Expand All @@ -196,6 +198,7 @@ async function execJupyter(
...cmd.slice(1),
resourcePath("jupyter/jupyter.py"),
],
// FIXME IS THIS NOT SET WITH THE DAEMON?
env: {
// Force default matplotlib backend. something simillar is done here:
// https://github.com/ipython/ipykernel/blob/d7339c2c70115bbe6042880d29eeb273b5a2e350/ipykernel/kernelapp.py#L549-L554
Expand All @@ -206,6 +209,7 @@ async function execJupyter(
// function within the notebook
"MPLBACKEND": "module://matplotlib_inline.backend_inline",
"PYDEVD_DISABLE_FILE_VALIDATION": "1",
...env,
},
stdout: "piped",
},
Expand Down
4 changes: 4 additions & 0 deletions src/execute/rmd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,8 @@ export const knitrEngine: ExecutionEngine = {

return output;
},
true,
options.env,
);
const includes = result.includes as unknown;
// knitr appears to return [] instead of {} as the value for includes.
Expand Down Expand Up @@ -259,6 +261,7 @@ async function callR<T>(
quiet?: boolean,
outputFilter?: (output: string) => string,
reportError = true,
env?: Record<string, string>,
): Promise<T> {
// establish cwd for our R scripts (the current dir if there is an renv
// otherwise the project dir if specified)
Expand Down Expand Up @@ -291,6 +294,7 @@ async function callR<T>(
...rscriptArgsArray,
resourcePath("rmd/rmd.R"),
],
env,
cwd,
stderr: quiet ? "piped" : "inherit",
},
Expand Down
1 change: 1 addition & 0 deletions src/execute/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export interface ExecutionTarget {

// execute options
export interface ExecuteOptions {
env: Record<string, string>;
target: ExecutionTarget;
format: Format;
resourceDir: string;
Expand Down
33 changes: 5 additions & 28 deletions src/quarto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,11 @@ const checkReconfiguration = async () => {
}
};

const passThroughPandoc = async (
args: string[],
env?: Record<string, string>,
) => {
const passThroughPandoc = async (args: string[]) => {
const result = await execProcess(
{
cmd: pandocBinaryPath(),
args: args.slice(1),
env,
},
undefined,
undefined,
Expand All @@ -95,10 +91,7 @@ const passThroughPandoc = async (
Deno.exit(result.code);
};

const passThroughTypst = async (
args: string[],
env?: Record<string, string>,
) => {
const passThroughTypst = async (args: string[]) => {
if (args[1] === "update") {
error(
"The 'typst update' command is not supported.\n" +
Expand All @@ -109,28 +102,26 @@ const passThroughTypst = async (
const result = await execProcess({
cmd: typstBinaryPath(),
args: args.slice(1),
env,
});
Deno.exit(result.code);
};

export async function quarto(
args: string[],
cmdHandler?: (command: Command) => Command,
env?: Record<string, string>,
) {
await checkReconfiguration();
checkVersionRequirement();
if (args[0] === "pandoc" && args[1] !== "help") {
await passThroughPandoc(args, env);
await passThroughPandoc(args);
}
if (args[0] === "typst") {
await passThroughTypst(args, env);
await passThroughTypst(args);
}

// passthrough to run handlers
if (args[0] === "run" && args[1] !== "help" && args[1] !== "--help") {
const result = await runScript(args.slice(1), env);
const result = await runScript(args.slice(1));
Deno.exit(result.code);
}

Expand All @@ -155,13 +146,6 @@ export async function quarto(

debug("Quarto version: " + quartoConfig.version());

const oldEnv: Record<string, string | undefined> = {};
for (const [key, value] of Object.entries(env || {})) {
const oldV = Deno.env.get(key);
oldEnv[key] = oldV;
Deno.env.set(key, value);
}

const quartoCommand = new Command()
.name("quarto")
.help({ colors: false })
Expand Down Expand Up @@ -191,13 +175,6 @@ export async function quarto(

try {
await promise;
for (const [key, value] of Object.entries(oldEnv)) {
if (value === undefined) {
Deno.env.delete(key);
} else {
Deno.env.set(key, value);
}
}
if (commandFailed()) {
exitWithCleanup(1);
}
Expand Down
25 changes: 13 additions & 12 deletions src/resources/jupyter/notebook.py
Original file line number Diff line number Diff line change
@@ -1,26 +1,24 @@
# pyright: reportMissingImports=false

import os
import re
import asyncio
import atexit
import base64
import copy
import glob
import sys
import json
import os
import pprint
import copy
import base64

import re
import sys
from pathlib import Path

from yaml import safe_load as parse_string
from yaml import safe_dump

from log import trace
import nbformat
from nbclient import NotebookClient
from jupyter_client import KernelManager
from jupyter_core_utils_vendor import run_sync
import asyncio
from log import trace
from nbclient import NotebookClient
from yaml import safe_dump
from yaml import safe_load as parse_string

# optional import of papermill for params support
try:
Expand Down Expand Up @@ -129,6 +127,8 @@ def set_env_vars(options):
else:
os.environ["QUARTO_FIG_DPI"] = str(options["fig_dpi"])
os.environ["QUARTO_FIG_FORMAT"] = options["fig_format"]
for key, value in options.get("env", {}).items():
os.environ[key] = value


def retrieve_nb_from_cache(nb, status, input, **kwargs):
Expand Down Expand Up @@ -201,6 +201,7 @@ def notebook_execute(options, status):
quiet = quarto_kernel_setup_options["quiet"]
resource_dir = quarto_kernel_setup_options["resource_dir"]
eval = quarto_kernel_setup_options["eval"]
quarto_kernel_setup_options["env"] = options.get("env", {})

# set environment variables
set_env_vars(quarto_kernel_setup_options)
Expand Down
14 changes: 14 additions & 0 deletions tests/docs/smoke-all/engine/env/julia.qmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
---
format: html
_quarto:
tests:
html:
ensureFileRegexMatches:
- ["julia.qmd"]
- []
engine: julia
---

```{julia}
ENV["QUARTO_DOCUMENT_FILE"]
```
16 changes: 16 additions & 0 deletions tests/docs/smoke-all/engine/env/jupyter.qmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
format: html
_quarto:
tests:
html:
ensureFileRegexMatches:
- ["jupyter.qmd"]
- []
---

```{python}
import os

print(os.environ.get("QUARTO_DOCUMENT_FILE", ""))
print("Hello, world")
```
13 changes: 13 additions & 0 deletions tests/docs/smoke-all/engine/env/knitr.qmd
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
---
format: html
_quarto:
tests:
html:
ensureFileRegexMatches:
- ["knitr.qmd"]
- []
---

```{r}
print(Sys.getenv("QUARTO_DOCUMENT_FILE"))
```
1 change: 1 addition & 0 deletions tests/smoke/env/install.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ testQuartoCmd(
[
noErrorsOrWarnings,
printsMessage({level: "INFO", regex: /tinytex\s+/}),
// FIXME WE SHOULD BE ABLE TO TURN THIS BACK ON
// printsMessage({level: "INFO", regex: /chromium\s+/}),
// temporarily disabled until we get puppeteer back
],
Expand Down
4 changes: 4 additions & 0 deletions tests/smoke/extensions/extension-render-journals.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,10 @@ for (const journalRepo of journalRepos) {
// Sets up the test
setup: async () => {
console.log(`using quarto-journals/${journalRepo.repo}`);

// FIXME THIS DOESN'T WORK
// WE CANNOT GUARANTEE THAT CHDIR WILL BE CONSTANT
// THROUGHOUT THE ASYNC CALL
const wd = Deno.cwd();
Deno.chdir(workingDir);
await quarto([
Expand Down
Loading
Loading