Add warnings as errors for ex_doc #8596

garazdawi · 2024-06-19T13:35:37Z

This PR adds a mechanism to scan the stderr output from ex_doc for warnings and if found fail the build.

github-actions · 2024-06-19T13:36:32Z

CT Test Results

5 files 191 suites 2h 11m 6s ⏱️
1 953 tests 1 893 ✅ 59 💤 1 ❌
2 714 runs 2 626 ✅ 87 💤 1 ❌

For more details on these failures, see this check.

Results for commit 75913d7.

♻️ This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

// Erlang/OTP Github Action Bot

chiroptical · 2024-06-19T18:48:14Z

make/ex_doc_wrapper.in

@@ -4,7 +4,7 @@ ARGS=("$@")

 set -eo pipefail {0}

-EX_DOC=$(command -v ex_doc)
+EX_DOC=$(command -v ex_doc || true)


I am getting "Could not find ex_doc!" because this is overriding EX_DOC.

EX_DOC=${EX_DOC:=$(command -v ex_doc || true)}

I just realized you are basically using EX_DOC to point to the wrapper.

Best case for the nix derivations is I can just define which EX_DOC to use directly and thread that into the wrapper.

chiroptical · 2024-06-19T18:48:44Z

make/ex_doc_wrapper.in

@@ -15,7 +15,7 @@ if [ -z "${EX_DOC}" ]; then
        if $ERL_TOP/otp_build download_ex_doc; then
            read -p "Press any key to continue..." -n 1 -r
            echo "continuing"
-            EX_DOC=$(command -v ex_doc)
+            EX_DOC=$(command -v ex_doc || true)


I think this is fine because EX_DOC wouldn't have been defined previously

chiroptical · 2024-06-19T18:56:59Z

Made some comments. Feel free to tag me if you want me to test again. Really appreciate you helping me with this!

(I don't expect you to do this) If for some reason you want to test yourself my branch is here https://github.com/chiroptical/nixpkgs/tree/test-ex-doc-updates. You would need nix and then run nix-build -A erlang_27. I am more than happy to do this because after the initial change works I want to remove some configuration from the builder and test that.

garazdawi · 2024-06-20T09:06:07Z

New attempt, you should be able to set EX_DOC=/path/to/ex_doc before running make or configure and it should use that ex_doc.

chiroptical · 2024-06-20T13:43:42Z

make/ex_doc.exs

@@ -151,7 +151,7 @@ current_datetime = System.os_time() |> DateTime.from_unix!(:native)
 config = [
  proglang: :erlang,
  source_url_pattern: source_url_pattern,
-  assets: Path.join(cwd, "/assets"),
+  assets: %{ Path.join(cwd, "/assets") => "assets" },


Next error I get from the nix build

warning: the VM is running with native name encoding of latin1 which may cause Elixir to malfunction as it expects utf8. Please ensure your locale is set to UTF-8 (which can be verified by running "locale" in your shell) or set the ELIXIR_ERL_OPTIONS="+fnu" environment variable ** (FunctionClauseError) no function clause matching in IO.chardata_to_string/1 The following arguments were given to IO.chardata_to_string/1: # 1 %{"/build/source/lib/stdlib/doc/assets" => "assets"}

stack trace

(elixir 1.16.3) lib/io.ex:687: IO.chardata_to_string/1 (elixir 1.16.3) lib/path.ex:662: Path.join/2 (ex_doc 0.32.1) lib/ex_doc/formatter/html.ex:294: ExDoc.Formatter.HTML.copy_assets/2 (ex_doc 0.32.1) lib/ex_doc/formatter/html.ex:288: ExDoc.Formatter.HTML.generate_assets/3 (ex_doc 0.32.1) lib/ex_doc/formatter/html.ex:25: ExDoc.Formatter.HTML.run/3 (ex_doc 0.32.1) lib/ex_doc/cli.ex:69: anonymous fn/6 in ExDoc.CLI.generate/3 (elixir 1.16.3) lib/enum.ex:1700: Enum."-map/2-lists^map/1-1-"/2 (elixir 1.16.3) lib/kernel/cli.ex:136: anonymous fn/3 in Kernel.CLI.exec_fun/2

I guess this comes from https://github.com/elixir-lang/ex_doc/blob/v0.34.0/CHANGELOG.md#v0340-2024-05-30? i.e. I think this requires a specific version of ex_doc now. Unsure if that is intended?

We will always require a specific version of ex_doc, which one is listed in https://github.com/erlang/otp/blob/master/make/ex_doc_vsn

Ahh, okay! Let me update that and keep testing thanks!

There are some bugs in v0.34.0 of ex_doc that cause some issues, so you may have to wait for v0.35.0 in order for everything to pass.

wojtekmach · 2024-06-20T14:02:07Z

@garazdawi as you may know we have an open PR that adds --warnings-as-errors to ExDoc (elixir-lang/ex_doc#1831) which I think would handle most if not all warnings that this PR catches. So if you'd be OK with relying on ExDoc for this instead we can try to push it over the finish line soon.

chiroptical · 2024-06-20T14:04:10Z

make/doc.mk

@@ -52,15 +52,17 @@ else
 DOC_TARGETS?=html
 endif

+EX_DOC_WARNINGS_AS_ERRORS?=true


Last question, should this be false by default?

I immediately had to turn it off in the nix build. Which worked by the way! Here is the simplification I was able to do with this https://github.com/chiroptical/nixpkgs/pull/4/files.

No I want it to be on by default as there should never be any warnings if you use the correct ExDoc vsn.

I get this from the build with that set to true.

warning: the VM is running with native name encoding of latin1 which may cause Elixir to malfunction as it expects utf8. Please ensure your locale is set to UTF-8 (which can be verified by running "locale" in your shell) or set the ELIXIR_ERL_OPTIONS="+fnu" environment variable

I guess you may want to set this in the wrapper then?

That would be something that needs fixing in nix I think. The Erlang VM thinks that filenames should be encoded in latin1 because the locale is not set to utf8.

The elixir nix package seems to set LANG and LC_TYPE to C.UTF-8 which should do the trick.

I guess so! Really appreciate you looking through all this.

garazdawi · 2024-06-20T16:51:42Z

@wojtekmach I would be happy to use a built-in ex_doc warnings as errors. I did this PR because a bunch of warnings slipped trough in Erlang/OTP and it did not seem like there was much interest in getting elixir-lang/ex_doc#1831 merged anytime soon.

chiroptical

From my standpoint, it looks great!

We check the output of ex_doc to see if there are any warnings emitted and if there are then we exit the build.

garazdawi added team:VM Assigned to OTP team VM feature labels Jun 19, 2024

garazdawi added this to the OTP-27.1 milestone Jun 19, 2024

garazdawi self-assigned this Jun 19, 2024

garazdawi mentioned this pull request Jun 19, 2024

Use built escript to execute ex_doc instead of one in PATH #8378

Closed

chiroptical reviewed Jun 19, 2024

View reviewed changes

garazdawi force-pushed the lukas/otp/fix-ex_doc-warnings-as-errors branch 2 times, most recently from af8fd65 to 80e418e Compare June 20, 2024 09:05

chiroptical reviewed Jun 20, 2024

View reviewed changes

chiroptical approved these changes Jun 20, 2024

View reviewed changes

chiroptical mentioned this pull request Jun 20, 2024

ex_doc: 0.32.1 -> 0.34.1 NixOS/nixpkgs#321367

Merged

13 tasks

garazdawi added 5 commits June 24, 2024 14:03

Fix ex_doc warnings

3f2487d

otp: Add support for warnings as errors for ex_doc

8d18ce9

We check the output of ex_doc to see if there are any warnings emitted and if there are then we exit the build.

Use escript to launch ex_doc for nix to work

3c3d26b

Read ex_doc from environment if set

0a5025e

Update gitignore

75913d7

garazdawi force-pushed the lukas/otp/fix-ex_doc-warnings-as-errors branch from 80e418e to 75913d7 Compare June 24, 2024 12:04

garazdawi added the testing currently being tested, tag is used by OTP internal CI label Jun 24, 2024

garazdawi merged commit d9f922e into erlang:master Jun 25, 2024
17 of 19 checks passed

chiroptical mentioned this pull request Jul 11, 2024

erlang: 25.3.2.12 -> 25.3.2.13; erlang_27: 27.0 -> 27.0.1 NixOS/nixpkgs#326403

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add warnings as errors for ex_doc #8596

Add warnings as errors for ex_doc #8596

garazdawi commented Jun 19, 2024

github-actions bot commented Jun 19, 2024 •

edited

Loading

chiroptical Jun 19, 2024

chiroptical Jun 19, 2024

chiroptical Jun 19, 2024

chiroptical commented Jun 19, 2024

garazdawi commented Jun 20, 2024

chiroptical Jun 20, 2024

chiroptical Jun 20, 2024

chiroptical Jun 20, 2024

garazdawi Jun 20, 2024

chiroptical Jun 20, 2024

garazdawi Jun 20, 2024

wojtekmach commented Jun 20, 2024

chiroptical Jun 20, 2024

garazdawi Jun 20, 2024

chiroptical Jun 20, 2024

garazdawi Jun 20, 2024

chiroptical Jun 20, 2024

garazdawi commented Jun 20, 2024

chiroptical left a comment

Add warnings as errors for ex_doc #8596

Add warnings as errors for ex_doc #8596

Conversation

garazdawi commented Jun 19, 2024

github-actions bot commented Jun 19, 2024 • edited Loading

CT Test Results

Artifacts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chiroptical commented Jun 19, 2024

garazdawi commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wojtekmach commented Jun 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garazdawi commented Jun 20, 2024

chiroptical left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 19, 2024 •

edited

Loading