Skip to content
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

Revert "Enable color output whenever stdout is TTY (#34347)" #35294

Merged
merged 1 commit into from
Mar 30, 2020

Conversation

maleadt
Copy link
Member

@maleadt maleadt commented Mar 29, 2020

Looks like #34347 broke PkgEval which tests packages using docker without a TTY:

tim@taurus ~/.cache/julia/binaries/nightly/x64$ docker run -it -v $(pwd):/opt/julia tutum/curl /opt/julia/julia -e 'using Pkg; Pkg.add("Example"); Pkg.test("Example")'                                                                                                       
    Cloning default registries into `~/.julia`
######################################################################## 100.0%
      Added registry `General` to `~/.julia/registries/General`
  Resolving package versions...
  Installed Example ─ v0.5.3
   Updating `~/.julia/environments/v1.5/Project.toml`
   7876af07 + Example v0.5.3
   Updating `~/.julia/environments/v1.5/Manifest.toml`
   7876af07 + Example v0.5.3
    Testing Example
     Status `~/tmp/jl_ULb6Wh/Project.toml`
   7876af07 Example v0.5.3
   8dfed614 Test
     Status `~/tmp/jl_ULb6Wh/Manifest.toml`
   7876af07 Example v0.5.3
   2a0f44e3 Base64
   8ba89e20 Distributed
   b77e0a4c InteractiveUtils
   56ddb016 Logging
   d6f4376e Markdown
   9a3f8284 Random
   9e88b42a Serialization
   6462fe0b Sockets
   8dfed614 Test
    Testing Example tests passed 

tim@taurus ~/.cache/julia/binaries/nightly/x64$ docker run -i -v $(pwd):/opt/julia tutum/curl /opt/julia/julia -e 'using Pkg; Pkg.add("Example"); Pkg.test("Example")'                                                                                                        
    Cloning default registries into `~/.julia`
######################################################################## 100.0%
      Added registry `General` to `~/.julia/registries/General`
  Resolving package versions...
  Installed Example ─ v0.5.3
   Updating `~/.julia/environments/v1.5/Project.toml`
   7876af07 + Example v0.5.3
   Updating `~/.julia/environments/v1.5/Manifest.toml`
   7876af07 + Example v0.5.3
    Testing Example
     Status `~/tmp/jl_QRq41T/Project.toml`
   7876af07 Example v0.5.3
   8dfed614 Test
     Status `~/tmp/jl_QRq41T/Manifest.toml`
   7876af07 Example v0.5.3
   2a0f44e3 Base64
   8ba89e20 Distributed
   b77e0a4c InteractiveUtils
   56ddb016 Logging
   d6f4376e Markdown
   9a3f8284 Random
   9e88b42a Serialization
   6462fe0b Sockets
   8dfed614 Test
ERROR: Package Example errored during testing
Stacktrace:
 [1] pkgerror(::String, ::Vararg{String,N} where N) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/Types.jl:53
 [2] test(::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}; coverage::Bool, julia_args::Cmd, test_args::Cmd, test_fn::Nothing) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/Operations.jl:1523
 [3] test(::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}; coverage::Bool, test_fn::Nothing, julia_args::Cmd, test_args::Cmd, kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:316
 [4] test(::Pkg.Types.Context, ::Array{Pkg.Types.PackageSpec,1}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:303
 [5] #test#68 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:297 [inlined]
 [6] test at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:297 [inlined]
 [7] #test#67 at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:296 [inlined]
 [8] test at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:296 [inlined]
 [9] test(::String; kwargs::Base.Iterators.Pairs{Union{},Union{},Tuple{},NamedTuple{(),Tuple{}}}) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:295
 [10] test(::String) at /buildworker/worker/package_linux64/build/usr/share/julia/stdlib/v1.5/Pkg/src/API.jl:295
 [11] top-level scope at none:1

As a result the daily PkgEval is broken: https://github.com/JuliaCI/NanosoldierReports/blob/master/pkgeval/by_date/2020-03/27/report.md

Adding --color=no fixes that:

tim@taurus ~/.cache/julia/binaries/nightly/x64$ docker run -i -v $(pwd):/opt/julia tutum/curl /opt/julia/julia --color=no -e 'using Pkg; Pkg.add("Example"); Pkg.test("Example")'                                                                                             
    Cloning default registries into `~/.julia`
######################################################################## 100.0%
      Added registry `General` to `~/.julia/registries/General`
  Resolving package versions...
  Installed Example ─ v0.5.3
   Updating `~/.julia/environments/v1.5/Project.toml`
   7876af07 + Example v0.5.3
   Updating `~/.julia/environments/v1.5/Manifest.toml`
   7876af07 + Example v0.5.3
    Testing Example
     Status `~/tmp/jl_QkNsGT/Project.toml`
   7876af07 Example v0.5.3
   8dfed614 Test
     Status `~/tmp/jl_QkNsGT/Manifest.toml`
   7876af07 Example v0.5.3
   2a0f44e3 Base64
   8ba89e20 Distributed
   b77e0a4c InteractiveUtils
   56ddb016 Logging
   d6f4376e Markdown
   9a3f8284 Random
   9e88b42a Serialization
   6462fe0b Sockets
   8dfed614 Test
    Testing Example tests passed

Opening a PR to run PkgEval and confirm.
@karan0299 could you have a look at why your changes cause this?

@maleadt maleadt added the DO NOT MERGE Do not merge this PR! label Mar 29, 2020
@maleadt
Copy link
Member Author

maleadt commented Mar 29, 2020

@nanosoldier runtests(["Example"], vs = ":master")

@nanosoldier
Copy link
Collaborator

Your package evaluation job has completed - no new issues were detected. A full report can be found here. cc @maleadt

@StefanKarpinski
Copy link
Member

Given the multiple issues, we should perhaps actually merge this revert.

@maleadt maleadt changed the title DO NOT MERGE: Revert "Enable color output whenever stdout is TTY (#34347)" Revert "Enable color output whenever stdout is TTY (#34347)" Mar 30, 2020
@maleadt maleadt removed the DO NOT MERGE Do not merge this PR! label Mar 30, 2020
@maleadt maleadt merged commit 12acd69 into master Mar 30, 2020
@maleadt maleadt deleted the tb/pkgeval_color branch March 30, 2020 07:35
@JeffBezanson
Copy link
Member

This seems to be because Pkg (as well as several other packages) uses this idiom:

        --color=$(Base.have_color ? "yes" : "no")

when spawning another julia process. This is very unfortunate. This is also supposed to be exactly what julia_cmd() is for. I see it is missing a couple flags, so I'll add them. I would also recommend exporting julia_cmd to encourage using that (not that it will help much, since most cases seem to use both julia_cmd and rolling their own option forwarding).

@KristofferC
Copy link
Member

I updated Pkg to be compatible with this change (JuliaLang/Pkg.jl#1746). But maybe it should just wait for #35324?

KristofferC added a commit that referenced this pull request Apr 7, 2020
This reverts commit 12acd69, reversing
changes made to 02624ac.
ztultrebor pushed a commit to ztultrebor/julia that referenced this pull request Apr 17, 2020
…color"

This reverts commit 12acd69, reversing
changes made to 02624ac.
staticfloat pushed a commit that referenced this pull request Apr 21, 2020
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.

5 participants