Add --warnings-as-errors flag for non-zero exit code (continued)

[eksperimental]

Closes #1411
Closes #1294

PR Originally published in #1412

I'm picking this feature up from it was left off.
The only thing to solve is the global state issue mentioned by @milmazz in #1412 (review)

[christhekeele]

I gave this a spin tonight, but noticed that my project (which produces ex_doc warnings) still returns with an exit code of 0.

Perhaps it's just that the types of warning that I have hasn't been instrumented yet? Repro details here.

[eksperimental]

Hi @christhekeele, I was working on this PR and I also noticed this due to some new tests failing.

[christhekeele]

I just read through the description of the global state issue. My idea for tackling it would be to:

• Convert ExDoc.WarningCounter from an Agent to an :ets table
  • so that new WarningCounters don't require modifications to a supervision tree
• Create a default :ets named table on ExDoc start for standard usage
  • ex: :ets.new(ExDoc.WarningCounter.Table, [:set, :named_table])
• Re-implement ExDoc.WarningCounter.increment
  • with :ets.update_counter/3
    • ex: :ets.update_counter(table, :count, 1, 0)
  • such that it looks for a table override in the process dictionary, but falls back to ExDoc.WarningCounter.Table
    • ex: Process.get(:ex_doc_warning_counter_table, ExDoc.WarningCounter.Table)
• Create a test helper macro that takes a block of code, and:
  • makes a new anonymous ets table
  • puts the pid in the process dictionary
  • executes the code
  • removes the pid from the process dictionary
  • deletes the table
• Use the macro where counter isolation is needed for the test

That should work.

[eksperimental]

Just wanted to let you guys know I am working on finishing this PR.

[christhekeele]

@eksperimental I'd forgotten this is the thread where I brainstormed the above approach above, but in fact ended up needing to do something similar last week for a test helper in another project! It worked well, and FWIW my solution essentially looks like what's suggested above, something like:

defmodule ExDoc.WarningCounter.TestHelpers do
  defmacro isolated_warning_counter(do: code) do
    quote location: :keep do
      temp_table = :ets.new([:set])
      Process.put({:ex_doc, :warnings_counter}, temp_table)

      try do
        unquote(code)
      after
        Process.delete({:ex_doc, :warnings_counter})
        :ets.delete(temp_table)
      end
    end
  end
end

defmodule ExDoc.WarningCounter do
  def increment do
    warnings_table = Process.get({:ex_doc, :warnings_counter}, ExDoc.WarningCounter)
  end
end

defmodule ExDoc.WarningCounterTest do
  use ExUnit
  import ExDoc.WarningCounter.TestHelpers

  test "some thing" do
    isolated_warning_counter do
      test_goes_here
    end
  end
end

The after ensures cleanup while preserving the return values/raise semantics of code. This enables per-file and even per-test parallelism without issue. The :ets.delete is probably overkill as it should disappear when the test process does, but prevents multiple stale tables sitting around if the isolated_warning_counter is invoked successfully many times in the same test.

If code spawns calls to ExDoc.WarningCounter.increment in other processes, though, it does fall apart.

[eksperimental]

Hi @christhekeele , thank you for the advice. Unfortunately I think it will not work because because ExDoc launches multiple processes, so working on a process level does not work.
I am looking at a solution similar to the macro in your last comment, where a value would be kept for the processes and sub-processes launched within that code block.

I can share what I have so far so you see the real code and how it fails.

[eksperimental]

@christhekeele I think we need to work on a caller level.
We can store a mapping of the caller pid and the ETS table (in my case it would be the counters ref since I am implementing it using the Erlang :counters module.

[christhekeele]

@eksperimental Sure, I'd love to see your code—though I don't notice any recent pushes to this branch. I can't quite visualize the callers solution you are proposing with something more concrete 😉

[eksperimental]

Hi @christhekeele
The solution is ready to be reviewed.
I think the implementation is complete, even though the CI failed in Elixir 1.14 / OTP 26.
I will have to look into it why it failed.

@elixir-lang/ex-doc There's one thing that is left. The error message says the docs failed to build due to warnings, which is not true. The docs are generated:
I think docs should not be generated. If that is the case we would have a temporarily build them in a folder inside the output folder, and on failure remove it..
Let me know what you guys think.

Also, what should the behavior be given mix docs uses generates epub and html, and one fail? Should non of them be created, or should they be judged independently.

[christhekeele]

Hey @eksperimental! Sorry for the delay, just got back from a vacation.

The error message says the docs failed to build due to warnings, which is not true. The docs are generated:
I think docs should not be generated. If that is the case we would have a temporarily build them in a folder inside the output folder, and on failure remove it...

I feel like this feature should go live and be tested in existing workflows before modifying the undocumented (outside of this error message) behaviour of what happens to artifacts during build, because I'm sure existing workflows out there implicitly rely on it. So I'd prefer we modify the error message be to be more accurate in the event of this failure mode, and propose a potentially-breaking temporary artifact management process after merge.

Also, what should the behavior be given mix docs uses generates epub and html, and one fail? Should non of them be created, or should they be judged independently.

I think we can defer this decision to after-merge based on real-world usage if we do the above.

The solution is ready to be reviewed.

I'd be happy to give the code a more detailed review, but cannot commit to finding the time for another few weeks, I'm afraid. Busy summer at work and in life!

If you want to test against the situation that led me to comment on your efforts to date, per this note, you should be able to do this in your shell:

git clone [email protected]:christhekeele/matcha.git
cd matcha
git checkout a4d02a9
# Edit this line:
# https://github.com/christhekeele/matcha/commit/a4d02a966c48233b244e0249a9ddeabaadc10b35#diff-dfa6f4ed74c90e5d4fda283d547d366586e690387289bcfd473e3fa5f9ace308R110
# to point the `ref` to your latest commit
mix docs --warnings-as-errors && echo success $? || echo failure $?
# Expect to see `failure 1`

[eksperimental]

Hi @christhekeele
Yes, the feature works as expected:

Doc generation failed due to warnings while using the --warnings-as-errors option
failure 20

We also need to review if the following warnings recently introduced to ExDoc, still qualify as warnings.
I wonder if they would be one level lower can be instead notices instead of warnings.

warning: ExDoc is outputting to an existing directory. Beware documentation output may be mixed with other entries
warning: file doc/api-reference.html already exists

[axelson]

I just tested this on https://github.com/ScenicFramework/scenic and it successfully caught the warning that currently exists on the main branch 🎉

> mix docs --warnings-as-errors
make: Nothing to be done for 'all'.
Compiling 43 files (.ex)
Generating docs...
warning: documentation references function "Scenic.Scene/push_script/4" but it is undefined or private
  lib/scenic/script.ex:7: Scenic.Script

View "html" docs at "doc/index.html"
Doc generation failed due to warnings while using the --warnings-as-errors option
[21:10:51] jason@linux-mint-desktop /mnt/arch_linux/home/jason/dev/forks/scenic [1]
> echo $status
1

Although I agree that the warnings are a little contradictory because the docs are still generated (which is counter to how --warnings-as-errors behaves when compiling elixir code).

[eksperimental]

May I ask what is needed to be complete with this PR?
I would really like to have this feature in ExDoc.

[josevalim]

Instead of introducing some global state, I wonder if you could create a counter and store it in the config. And then you call warning(config, ...) which will bump the counter. This means it is naturally async and you don't need the additional process. :)

[wojtekmach]

In the same vein, perhaps mix docs would return https://hexdocs.pm/mix/1.15/Mix.Task.Compiler.Diagnostic.html structs? Could be useful for IDEs? Or since ExDoc is not a compiler it’s a bad idea?

[josevalim]

Yeah, I don't see how they could be used if it is not a compiler :(

[eksperimental]

Passing config simplifies a lot the implementation.
The problem I am facing now is that there are functions that generate a warning and have no notion of config,
such as ExDoc.Markdown.Earmark.to_ast/2

[josevalim]

We would need to either:

1. pass an optional function to them to configure how logging happens
2. have those functions return their warnings so we can emit them

[wojtekmach]

I’d do the latter, return {ast, %{warnings: warnings}} or something like that.

I think such APIs that return instead of emit warnings are nice because we can test without capture_io and thus async. In this particular case since it’s not useful to return these warnings from mix docs, what we can also do is push all ExDoc warnings via some ExDoc.Utils.warn, inside call Application.put_env(:ex_doc, :warnings_emitted, true), check for this app env at the end of the run and call it a day. :)

[josevalim]

The counter will tell us if warnings were emitted or not. So having a config plus function return should be enough!

[eksperimental]

I will work on this solution and let you know when it is done.

[josevalim]

Thank you ❤️

[wojtekmach]

This PR now has a bunch of conflicts because we've made a lot of internal changes. Sorry about that!

On the flip side, I believe implementing this feature should be now much easier.

I have added a ExDoc.Utils.warn/2 function.

One way to go about it is:

1. add another utility function:

defmodule ExDoc.Utils do
  def warned? do
    :persistent_term.get({__MODULE__, :warned?}, false)
  end
end

2. add to warn/2:

unless warned?() do
  :persistent_term.put({__MODULE__, :warned?}, true)
end

3. In ExDoc.CLI check if --warnings-as-errors was passed and if so and warned? returns true, fail.

PR welcome!

[eksperimental]

The initial solution had a global state and it was simple.
I tried implementing the suggestion of not having a global state and it was too much that had to be done just to please the tests and in the end I never managed to finished it due to the amount of work.

I will re-implement it using :persistent_term.

[warmwaffles]

I was actually just looking for this option again to throw into our CI pipeline to make sure we keep everything up to date.

[eksperimental]

✅ Done: #1831