Skip to content

shell: Allow duplicating output fds (e.g. 2>&1) - #9004

Merged
Jarred-Sumner merged 12 commits into
mainfrom
shell-dupe-out-fd
Feb 22, 2024
Merged

Jarred-Sumner merged 12 commits into
mainfrom
shell-dupe-out-fd

Conversation

@zackradisic

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR enables support for the "Duplicating an Output File Descriptor" syntax (e.g. 2>&1)

Only the stdout/stderr fds are supported right now. So you can redirect stderr to stdout 2>&1, or redirect stdout to stderr 1>&2

  • Code changes

How did you verify your code works?

Zig files changed:

  • I checked the lifetime of memory allocated to verify it's (1) freed and (2) only freed when it should be
  • I included a test for the new code, or an existing test covers it
  • JSValue used outside outside of the stack is either wrapped in a JSC.Strong or is JSValueProtect'ed
  • I wrote TypeScript/JavaScript tests and they pass locally (bun-debug test test-file-name.test)

@github-actions

github-actions Bot commented Feb 20, 2024 •

Copy link
Copy Markdown
Contributor

✅ test failures on bun-darwin-aarch64 have been resolved.

#8082dab68d48a625f75cfa16cf376f1f8f269f9f

@github-actions

github-actions Bot commented Feb 20, 2024 •

Copy link
Copy Markdown
Contributor

❌ @zackradisic 1 files with test failures on linux-x64:

View test output

#8082dab68d48a625f75cfa16cf376f1f8f269f9f

@github-actions

github-actions Bot commented Feb 20, 2024 •

Copy link
Copy Markdown
Contributor

❌ @zackradisic 2 files with test failures on linux-x64-baseline:

View test output

#8082dab68d48a625f75cfa16cf376f1f8f269f9f

@github-actions

github-actions Bot commented Feb 20, 2024 •

Copy link
Copy Markdown
Contributor

❌🪟 @zackradisic, there are 18 test regressions on Windows x86_64

  • test\bundler\bundler_edgecase.test.ts
  • test\cli\hot\hot.test.ts
  • test\cli\run\require-cache.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\dns\resolve-dns.test.ts
  • test\js\bun\http\fetch-file-upload.test.ts
  • test\js\bun\shell\shelloutput.test.ts
  • test\js\bun\shell\throw.test.ts
  • test\js\deno\fetch\response.test.ts
  • test\js\bun\http\bun-server.test.ts
  • test\js\node\dns\node-dns.test.js
  • test\js\node\process\process.test.js
  • test\js\web\fetch\body.test.ts
  • test\js\web\fetch\body-stream.test.ts
  • test\js\web\fetch\fetch.test.ts
  • test\js\web\timers\setTimeout.test.js
  • test\js\web\websocket\websocket.test.js
  • test\js\web\workers\worker.test.ts

Full Test Output

Comment thread src/shell/strsearch.zig Outdated
const std = @import("std");
const bun = @import("root").bun;

pub fn search(str: anytype, needle: anytype) ?usize {}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tihs is dead code

@Jarred-Sumner

Copy link
Copy Markdown
Collaborator

Can you add this to the docs to explain its supported? Along with stdin redirection

@github-actions

github-actions Bot commented Feb 20, 2024 •

Copy link
Copy Markdown
Contributor

❌ @zackradisic 1 files with test failures on bun-darwin-x64:

View test output

#8082dab68d48a625f75cfa16cf376f1f8f269f9f

Comment thread docs/runtime/shell.md Outdated
- `Bun.file(path)`, `Bun.file(fd)` (reads from the file)
- `Response` (reads from the body)

### To/From Files

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a section on Redirects?

And then

Redirecting stdout → file

Redirecting stderr → file

Redirecting stdin ← file

Redirecting stdout → stderr

Redirecting stdout ← stderr

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

basically, people might not immediately make the mental leap that stdout and stderr are a "file"

Also, we could do buffers in there too.

Comment thread src/shell/subproc.zig
@@ -1463,6 +1465,7 @@ pub fn NewShellSubprocess(comptime EventLoopKind: JSC.EventLoopKind, comptime Sh
spawn_args.stdio[2].setUpChildIoPosixSpawn(
&actions,
stderr_pipe,

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stderr_pipe,
stderr_pipe


await TestBuilder.command`echo foo bar > file.txt; cat < file.txt`.ensureTempDir().stdout("foo bar\n").run();

await TestBuilder.command`BUN_DEBUG_QUIET_LOGS=1 ${BUN} -e ${"console.log('Stdout'); console.error('Stderr')"} 2>&1`

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove BUN_DEBUG_QUIET_LOGS from these, it should do that automatically now

@Jarred-Sumner
Jarred-Sumner merged commit 2605722 into main Feb 22, 2024
@Jarred-Sumner
Jarred-Sumner deleted the shell-dupe-out-fd branch February 22, 2024 02:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants