mirror of
https://github.com/ethanolivertroy/obsidian-markitdown.git
synced 2026-07-22 05:41:54 +00:00
Address review feedback before merge
Probe modern pipx homes, buffer install stdout for JSON, and clean up failed venv creates so Install Markitdown retries reliably. Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
parent
aac393071f
commit
f74856b711
6 changed files with 77 additions and 31 deletions
23
main.js
23
main.js
File diff suppressed because one or more lines are too long
|
|
@ -16,6 +16,7 @@ Output:
|
|||
import argparse
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
|
|
@ -35,7 +36,7 @@ def run_pip(python: str, package: str) -> int:
|
|||
bufsize=1,
|
||||
)
|
||||
|
||||
assert process.stdout is not None
|
||||
if process.stdout is not None:
|
||||
for line in process.stdout:
|
||||
print(line, end="", flush=True)
|
||||
|
||||
|
|
@ -55,6 +56,8 @@ def ensure_venv(venv_dir: str, bootstrap_python: str) -> str:
|
|||
text=True,
|
||||
)
|
||||
if result.returncode != 0 or not os.path.isfile(py):
|
||||
# Partial/failed create can leave a non-empty dir that blocks retries
|
||||
shutil.rmtree(venv_dir, ignore_errors=True)
|
||||
detail = (result.stderr or result.stdout or "").strip()
|
||||
raise RuntimeError(
|
||||
f"Failed to create virtual environment at {venv_dir}: {detail or f'exit {result.returncode}'}"
|
||||
|
|
|
|||
|
|
@ -386,7 +386,10 @@ export class SettingsTab extends PluginSettingTab {
|
|||
});
|
||||
|
||||
const pipxCmd = 'pipx install "markitdown[all]"';
|
||||
const pipCmd = `${this.plugin.resolvedPythonPath} -m pip install "markitdown[all]"`;
|
||||
const pythonPath = this.plugin.resolvedPythonPath;
|
||||
const quotedPython =
|
||||
pythonPath.includes(' ') ? `"${pythonPath}"` : pythonPath;
|
||||
const pipCmd = `${quotedPython} -m pip install "markitdown[all]"`;
|
||||
|
||||
for (const cmd of [pipxCmd, pipCmd]) {
|
||||
const cmdContainer = content.createDiv('markitdown-pip-command');
|
||||
|
|
|
|||
|
|
@ -95,26 +95,35 @@ describe('buildPythonCandidates', () => {
|
|||
const pluginDir = '/plugins/markitdown';
|
||||
const originalPlatform = process.platform;
|
||||
const originalHome = process.env.HOME;
|
||||
const originalUserProfile = process.env.USERPROFILE;
|
||||
const originalLocalAppData = process.env.LOCALAPPDATA;
|
||||
const originalPipxHome = process.env.PIPX_HOME;
|
||||
|
||||
afterEach(() => {
|
||||
Object.defineProperty(process, 'platform', { value: originalPlatform });
|
||||
if (originalHome === undefined) {
|
||||
delete process.env.HOME;
|
||||
} else {
|
||||
process.env.HOME = originalHome;
|
||||
}
|
||||
delete process.env.PIPX_HOME;
|
||||
const restore = (key: string, value: string | undefined) => {
|
||||
if (value === undefined) delete process.env[key];
|
||||
else process.env[key] = value;
|
||||
};
|
||||
restore('HOME', originalHome);
|
||||
restore('USERPROFILE', originalUserProfile);
|
||||
restore('LOCALAPPDATA', originalLocalAppData);
|
||||
restore('PIPX_HOME', originalPipxHome);
|
||||
});
|
||||
|
||||
it('includes plugin venv and pipx python on darwin', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin' });
|
||||
process.env.HOME = '/Users/test';
|
||||
delete process.env.PIPX_HOME;
|
||||
|
||||
const paths = buildPythonCandidates('python', pluginDir);
|
||||
|
||||
expect(paths[0]).toBe('python');
|
||||
expect(paths).toContain(getPluginVenvPython(pluginDir));
|
||||
expect(paths).toContain('/Users/test/.local/pipx/venvs/markitdown/bin/python');
|
||||
expect(paths).toContain(
|
||||
'/Users/test/Library/Application Support/pipx/venvs/markitdown/bin/python'
|
||||
);
|
||||
expect(paths).toContain('/opt/homebrew/bin/python3');
|
||||
expect(paths).toContain('/opt/homebrew/bin/python3.11');
|
||||
expect(paths).toContain('/opt/homebrew/opt/python@3.11/bin/python3');
|
||||
|
|
@ -124,6 +133,7 @@ describe('buildPythonCandidates', () => {
|
|||
it('still adds fallbacks when configured path is absolute', () => {
|
||||
Object.defineProperty(process, 'platform', { value: 'darwin' });
|
||||
process.env.HOME = '/Users/test';
|
||||
delete process.env.PIPX_HOME;
|
||||
|
||||
const paths = buildPythonCandidates('/opt/homebrew/bin/python3', pluginDir);
|
||||
|
||||
|
|
@ -146,6 +156,7 @@ describe('buildPythonCandidates', () => {
|
|||
process.env.HOME = 'C:\\Users\\test';
|
||||
process.env.USERPROFILE = 'C:\\Users\\test';
|
||||
process.env.LOCALAPPDATA = 'C:\\Users\\test\\AppData\\Local';
|
||||
delete process.env.PIPX_HOME;
|
||||
|
||||
const paths = buildPythonCandidates('python', pluginDir);
|
||||
|
||||
|
|
|
|||
|
|
@ -57,6 +57,7 @@ const INSTALL_PACKAGE_PY = `#!/usr/bin/env python3
|
|||
import argparse
|
||||
import json
|
||||
import os
|
||||
import shutil
|
||||
import subprocess
|
||||
import sys
|
||||
|
||||
|
|
@ -69,6 +70,7 @@ def run_pip(python, package):
|
|||
process = subprocess.Popen(
|
||||
[python, "-m", "pip", "install", package],
|
||||
stdout=subprocess.PIPE, stderr=subprocess.STDOUT, text=True, bufsize=1)
|
||||
if process.stdout is not None:
|
||||
for line in process.stdout:
|
||||
print(line, end="", flush=True)
|
||||
process.wait()
|
||||
|
|
@ -83,6 +85,7 @@ def ensure_venv(venv_dir, bootstrap_python):
|
|||
[bootstrap_python, "-m", "venv", venv_dir],
|
||||
capture_output=True, text=True)
|
||||
if result.returncode != 0 or not os.path.isfile(py):
|
||||
shutil.rmtree(venv_dir, ignore_errors=True)
|
||||
detail = (result.stderr or result.stdout or "").strip()
|
||||
raise RuntimeError(
|
||||
f"Failed to create virtual environment at {venv_dir}: {detail or f'exit {result.returncode}'}")
|
||||
|
|
|
|||
|
|
@ -79,9 +79,24 @@ export function buildPythonCandidates(pythonPath: string, pluginDir: string): st
|
|||
const home = process.env.HOME || process.env.USERPROFILE || '';
|
||||
const isWin = process.platform === 'win32';
|
||||
|
||||
// pipx-managed markitdown (isolated venv; system python3 cannot import it)
|
||||
const pipxHome = process.env.PIPX_HOME || (home ? path.join(home, '.local', 'pipx') : '');
|
||||
if (pipxHome) {
|
||||
// pipx-managed markitdown (isolated venv; system python3 cannot import it).
|
||||
// Probe PIPX_HOME plus legacy (~/.local/pipx) and current platformdirs defaults.
|
||||
const pipxHomes: string[] = [];
|
||||
if (process.env.PIPX_HOME) {
|
||||
pipxHomes.push(process.env.PIPX_HOME);
|
||||
}
|
||||
if (home) {
|
||||
pipxHomes.push(path.join(home, '.local', 'pipx'));
|
||||
if (isWin) {
|
||||
const localAppData = process.env.LOCALAPPDATA || path.join(home, 'AppData', 'Local');
|
||||
pipxHomes.push(path.join(localAppData, 'pipx'));
|
||||
} else if (process.platform === 'darwin') {
|
||||
pipxHomes.push(path.join(home, 'Library', 'Application Support', 'pipx'));
|
||||
} else {
|
||||
pipxHomes.push(path.join(home, '.local', 'share', 'pipx'));
|
||||
}
|
||||
}
|
||||
for (const pipxHome of pipxHomes) {
|
||||
if (isWin) {
|
||||
pathsToTry.push(path.join(pipxHome, 'venvs', 'markitdown', 'Scripts', 'python.exe'));
|
||||
} else {
|
||||
|
|
@ -295,13 +310,16 @@ export async function installPackage(
|
|||
}
|
||||
);
|
||||
|
||||
let lastLine = '';
|
||||
let stdoutBuf = '';
|
||||
let lastCompleteLine = '';
|
||||
|
||||
child.stdout.on('data', (data: Buffer) => {
|
||||
const lines = data.toString().split('\n');
|
||||
stdoutBuf += data.toString();
|
||||
const lines = stdoutBuf.split('\n');
|
||||
stdoutBuf = lines.pop() ?? '';
|
||||
for (const line of lines) {
|
||||
if (line.trim()) {
|
||||
lastLine = line.trim();
|
||||
lastCompleteLine = line.trim();
|
||||
onProgress?.(line.trim());
|
||||
}
|
||||
}
|
||||
|
|
@ -321,9 +339,14 @@ export async function installPackage(
|
|||
});
|
||||
|
||||
child.on('close', (code: number | null) => {
|
||||
const trailing = stdoutBuf.trim();
|
||||
if (trailing) {
|
||||
lastCompleteLine = trailing;
|
||||
onProgress?.(trailing);
|
||||
}
|
||||
if (code === 0) {
|
||||
try {
|
||||
const result = JSON.parse(lastLine);
|
||||
const result = JSON.parse(lastCompleteLine);
|
||||
resolve(result.success === true);
|
||||
} catch {
|
||||
resolve(true);
|
||||
|
|
|
|||
Loading…
Reference in a new issue