Skip to content
This repository has been archived by the owner on Sep 30, 2024. It is now read-only.

dev/sg: Disable forced TTY and forced color in NewOutput #42017

Merged
merged 2 commits into from
Sep 27, 2022

Conversation

marekweb
Copy link
Contributor

@marekweb marekweb commented Sep 23, 2022

Fix for the error message seen in #41838 #39687 #41507 and #42136 when running sg when stdout is not a terminal (e.g. when piping to another comand).

❗️ An error was returned when detecting the terminal size and capabilities:
   
   GetWinsize: inappropriate ioctl for device

Disabling forceTTY allows terminal detection to work and avoids calling GetWinSize on a non-TTY.

Also disable forceColor based on the same rationale.

Worked on during sg hack hour with @mucles and @danieldides

Test plan

Before

go run ./dev/sg version | less -r
❗️ An error was returned when detecting the terminal size and capabilities:
   
   GetWinsize: inappropriate ioctl for device
   
   Execution will continue, but please report this, along with your operating
   system, terminal, and any other details, to:
     https://github.com/sourcegraph/sourcegraph/issues/new
   
dev

After

go run ./dev/sg version | less -r
dev

@cla-bot cla-bot bot added the cla-signed label Sep 23, 2022
@marekweb marekweb added team/devx sg sg, the Sourcegraph developer command labels Sep 23, 2022
@marekweb marekweb requested a review from a team September 23, 2022 17:03
@marekweb marekweb marked this pull request as ready for review September 23, 2022 17:03
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Sep 23, 2022

Codenotify: Notifying subscribers in OWNERS files for diff 7f29865...e29d4b2.

Notify File(s)
@mrnugget dev/sg/internal/std/output.go
@sourcegraph/dev-experience dev/sg/internal/std/output.go

Copy link
Contributor

@mrnugget mrnugget left a comment

Choose a reason for hiding this comment

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

Did you manually confirm that sg lint, sg setup, etc. all still work?

I also wonder whether this wasn't used for the integration tests. If so, an env war should probably be used here to overwrite the behaviour in tests.

dev/sg/internal/std/output.go Outdated Show resolved Hide resolved
@marekweb
Copy link
Contributor Author

Did you manually confirm that sg lint, sg setup, etc. all still work?

Yes. Also: colors are working in Buildkite logs because, I think, Buildkite stdout emulates a terminal, so it looks like that's still working as intended.

I also wonder whether this wasn't used for the integration tests. If so, an env war should probably be used here to overwrite the behaviour in tests.

I saw that we have SG_DISABLE_OUTPUT_DETECTION which is intended to enable forceTTY and forceColor.

https://github.com/sourcegraph/sourcegraph/blob/9c6b6e2b43865ecc568d7c139dcc9b128ab122c3/dev/sg/main.go#L134-L138

Do you know more about how integration tests might be affected? Would this existing env var cover our needs there?

@mrnugget
Copy link
Contributor

Do you know more about how integration tests might be affected? Would this existing env var cover our needs there?

If the tests pass, then everything should be fine :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed sg sg, the Sourcegraph developer command
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants