Repository navigation
OMNIS-35: Keep package manager output out of read-only commands - #41
Merged
Merged
Conversation
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
YouTrack: https://youtrack.xn--wrm-hoa.li/issue/OMNIS-35
omnishell list,diff/apply --dry-runanddoctorprinted the package manager's answers (for example thefzf 0.74.4line frombrew list --versions fzf) into their own output. On hosts where the package manager writes to stdout,omnishell list --jsonwas therefore not valid JSON: it started with one stray line per probed package, followed by the JSON.Cause
pkgmgr.ExecRunnercopies the output of every command it runs to the writersbuildEnginewas given, which are the process's real stdout and stderr. That is right for installs and hooks, whose output the user is waiting for, but wrong for the read-only "is this package installed?" probes. The same writers are also the engine's own message writers (warning: unknown module ..., the plan text), so simply passingio.Discardwould have swallowed those too.Changes
internal/cli/context.go:newEnginetakes separate writers for the engine and for its runner.buildEnginebehaves as before. NewbuildQueryEnginekeeps the engine's messages on stdout and stderr and drops the runner's output.list,doctor(without--fix),diffandapply --dry-runusebuildQueryEngine;runApplyreads--dry-runbefore it builds the engine.apply,doctor --fix,removeanduninstallstill stream installation and hook output.validateandbenchdo not ask the package manager anything.CHANGELOG.md: entry underFixed.Commits
Test plan
internal/cli/quiet_test.go, written first and failing for the right reason:list --jsonis valid JSON and the table has no foreign lines even when the runner writes to the writer it is given; the runner getsio.Discardforlist,list --json,diff,apply --dry-runanddoctor, and the real writers forapplyanddoctor --fix; the engine's unknown-module warning still reaches stderr.brewonPATHthat prints one line per call: before the changelist --jsonstarted with 15 stray lines, afterwards it parses (22 modules).make vetandmake test(race detector) pass;golangci-lintv2.13.2 (the CI version, viago run) reports 0 issues;go.modandgo.sumare unchanged.golangci-lint,govulncheckand the build matrix pass.omnishell list --json | python3 -m json.toolandomnishell diff; neither may show package manager lines.Open question (not part of this fix)
A real
applystill streams everything the runner prints, including the probe answers between the plan and the installation. Probes and installs go through the same runner and writers, so they cannot be separated without changing theRunnerinterface; this PR leaves that alone.