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

Set CLICOLOR_FORCE=0 #6608

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Set CLICOLOR_FORCE=0 #6608

merged 2 commits into from
Nov 30, 2022

Conversation

emillon
Copy link
Collaborator

@emillon emillon commented Nov 30, 2022

Since #6340 we're considering CLICOLOR_FORCE to determine whether stderr supports color. However this changes behavior observable from the test suite, so CLICOLOR_FORCE=1 make test fails (this is tracked as #6607).

The issue is that ocaml/setup-ocaml#631, this variable is set in the CI environment. So we disable it until the situation is fixed (and this variable does not have observable changes anymore).

@MisterDA
Copy link
Contributor

MisterDA commented Nov 30, 2022

IMO that's a good patch.
I would say as an analogy that if Dune supported translations, you'd probably select a single language for expect tests that you'd set in the environment like this, so it makes sense to me to "reset" the environment in such a way.
Nevertheless, my apologies for not anticipating that problem in my previous PRs.

@rgrinberg
Copy link
Member

Looks OK, but should we set this variable for our expect, cram, and mdx rules? Otherwise, I predict that our users will run into the same problem.

@emillon
Copy link
Collaborator Author

emillon commented Nov 30, 2022

Yeah this PR is solely to fix our own CI, it doesn't mark the issue as closed. We still need to address the rest of the issue.

@rgrinberg
Copy link
Member

Sounds good. How about we then:

  1. Make a note that this is a temporary workaround.
  2. Add some tests for the underlying issue. For example, a cram test that runs a color output binary under dune with the variable set.

Since ocaml#6340 we're considering `CLICOLOR_FORCE` to determine whether
stderr supports color. However this changes behavior observable from the
test suite, so `CLICOLOR_FORCE=1 make test` fails (this is tracked as
ocaml#6607).

The issue is that ocaml/setup-ocaml#631, this variable is set in the CI
environment. So we disable it until the situation is fixed (and this
variable does not have observable changes anymore).

Signed-off-by: Etienne Millon <[email protected]>
Signed-off-by: Etienne Millon <[email protected]>
@emillon
Copy link
Collaborator Author

emillon commented Nov 30, 2022

OK! I'm adding the comment here and merging it to make CI green, and will add tests in a separate PR.

@emillon emillon merged commit a093bd0 into ocaml:main Nov 30, 2022
@emillon emillon deleted the clicolor-force-disable branch November 30, 2022 17:33
moyodiallo pushed a commit to moyodiallo/dune that referenced this pull request Dec 2, 2022
* Set CLICOLOR_FORCE=0

Since ocaml#6340 we're considering `CLICOLOR_FORCE` to determine whether
stderr supports color. However this changes behavior observable from the
test suite, so `CLICOLOR_FORCE=1 make test` fails (this is tracked as
ocaml#6607).

The issue is that ocaml/setup-ocaml#631, this variable is set in the CI
environment. So we disable it until the situation is fixed (and this
variable does not have observable changes anymore).

Signed-off-by: Etienne Millon <[email protected]>
jchavarri added a commit to jchavarri/dune that referenced this pull request Dec 5, 2022
* main: (54 commits)
  doc: how we write `to_dyn` and `equal` (ocaml#6621)
  test(cache): test output of man pages
  test: dune utop for (subdir ..) (ocaml#6629)
  refactor: improve style of utop rules (ocaml#6628)
  test: depend on utop (ocaml#6627)
  refactor(stdune): Add Appendable_list.cons (ocaml#6624)
  doc: tighten wording in README.md
  test: add a repro case for ocaml#6607 (ocaml#6612)
  doc: cleanup status badges in README.md (ocaml#6618)
  doc(README): rewrap paragraphs and cleanup links
  coq: more resilient config query
  fix: link time code gen (ocaml#6606)
  fix(melange): run melc ppx with merlin (ocaml#6476)
  feature(melange): add compile_flags (ocaml#6569)
  refactor: move `modules: Modules.t` from `Dune_package.Lib` to `Lib_info` (ocaml#6605)
  Set CLICOLOR_FORCE=0 (ocaml#6608)
  fix: declare deps for ccomp detection (ocaml#6610)
  refactor: assume Cmdliner.Arg.conv is abstract (ocaml#6609)
  refactor: gen_rules pattern matching (ocaml#6604)
  Add CI for MSVC using dkml-workflows (ocaml#6540)
  ...
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.

3 participants