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

Execute OTP-specific ex_doc (from priv) #83

Merged

Conversation

paulo-ferraz-oliveira
Copy link
Collaborator

@paulo-ferraz-oliveira paulo-ferraz-oliveira commented Feb 22, 2024

We add OTP-specific ex_doc binary execution.

⚠️ I'm basing this on top of #80, since I wanna benefit from the changes done there, and then do some. When that one's merged GitHub auto-rebases this one. (that would've happened if I had committed directly to this repo., which wasn't the case 😄)
⚠️ We need to wait until ex_doc is available for OTP 27 (or maybe as a fallback, we can assume Erlang maintains compatibility for at least one version and always try that with a warning?).

Note

Individual commit messages contain more detail on what was done and part of the motivation for these change.

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the feature/broader-otp-support branch 5 times, most recently from 21618bc to 02dc876 Compare February 22, 2024 02:33
@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as draft February 22, 2024 02:42
@garazdawi
Copy link

Since the ex_doc built for 26 is guaranteed to work with 27 and 28, there is no need wait for a 27 build to be sure things work with 27.

I imagine the ex_doc team will start producing 27 ex_doc binaries when the final release is done. Otherwise you run the risk of a backward incompatible fix in a release candidate will break the old builds.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Feb 24, 2024

Something's failing with the output generated on OTP 27.0-rc1. @starbelly, ideas? @garazdawi, if you have a little time to spare and this rings a bell, thoughts would be appreciated. Thanks.

Edit: fwiw, results are stuff like

%%% rebar3_ex_doc_SUITE ==> generate_docs_overriding_output_set_in_config: FAILED
%%% rebar3_ex_doc_SUITE ==>
Failure/Error: ?assertMatch({ match , [ << "foo/0 does nothing" >> ] }, re : run ( ModuleDoc , "foo/0 does nothing" , [ { capture , [ 0 ] , binary } ] ))
  expected: = { match , [ << "foo/0 does nothing" >> ] }
       got: nomatch
      line: 239

which seems to indicate (as per the tests' current expectations) something's off in generation. ... later... I think I see it, the expected input is based on new directives and not backward compatible with EDoc. 😄

@paulo-ferraz-oliveira paulo-ferraz-oliveira marked this pull request as ready for review February 24, 2024 01:16
@paulo-ferraz-oliveira
Copy link
Collaborator Author

f1182e1 fixes compilation and checks on OTP 27: we're expected to use -moduledoc and -doc.

At the same time, I decided to use if to distinguish from OTP releases (I think it's easier to find and handle these when update time comes, than to reason on otp_release).

@paulo-ferraz-oliveira paulo-ferraz-oliveira force-pushed the feature/broader-otp-support branch 5 times, most recently from b3876b5 to 18f104b Compare February 26, 2024 00:47
Comment on lines 48 to 59
- otp: '24'
elixir: '1.13'
cache-id: oldest
- otp: '25'
elixir: '1.14'
cache-id: plus_one
- otp: '26'
elixir: '1.15'
cache-id: plus_two
- otp: '27.0-rc1'
elixir: '1.16'
cache-id: plus_three
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I save a bunch of caches, identified by cache-id...

Comment on lines 89 to 115
- name: Restore cache (oldest) # We do this to use the file in a non-Elixir env.
uses: actions/cache/restore@v4
with:
path: priv
key: consumer-cache-oldest
- name: Restore cache (plus_one)
uses: actions/cache/restore@v4
with:
path: priv
key: consumer-cache-plus_one
- name: Restore cache (plus_two)
uses: actions/cache/restore@v4
with:
path: priv
key: consumer-cache-plus_two
- name: Restore cache (plus_three)
uses: actions/cache/restore@v4
with:
path: priv
key: consumer-cache-plus_three
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

... and then recover them for execution. Ofc, at this particular moment the names are "fixed" to a four version window, but this might change/be tweaked.

@@ -351,11 +352,27 @@ get_app_metadata(Name, Vsn) ->
{links, []}
]}.

-if(?OTP_RELEASE >= 27).

Choose a reason for hiding this comment

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

Both edoc and doc attributes should work in 27, so maybe you want to test with both?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, we can do that. It's just that when I did @doc, as previously, it didn't generate the same output.

Module under test was

        "%%%-------------------------------------------------------------------\n"
        "%% @doc `~s' - a module\n"
        "%% @end\n"
        "%%%-------------------------------------------------------------------\n"
        "-module('~s').\n"
        "-export([foo/0]).\n"
        "%%% @doc\n"
        "%%% foo/0 does nothing\n"
        "%%% @end\n"
        "-spec foo() -> ok.\n"
        "foo() -> ok.\n",
        [Name, Name]

and there was no generation for foo/0 does nothing, unless that one's supposed to be pure EDoc, and not rebar3_ex_doc.

Choose a reason for hiding this comment

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

You need to pass +nodocs when compiling the file, otherwise it will create empty doc entries in the beam file and ex_doc will use those instead of the .chunk file that edoc creates.

        "%%%-------------------------------------------------------------------\n"
        "%% @doc `~s' - a module\n"
        "%% @end\n"
        "%%%-------------------------------------------------------------------\n"
        "-module('~s').\n"
        "-compile(nodocs).\n"
        "-export([foo/0]).\n"
        "%%% @doc\n"
        "%%% foo/0 does nothing\n"
        "%%% @end\n"
        "-spec foo() -> ok.\n"
        "foo() -> ok.\n",

Come to think of it, that is probably something we could and should warn for in edoc_layout_chunks....

Choose a reason for hiding this comment

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

Actually, I will fix it so that if there are no doc attributes in the code at all, it will be the same as passing nodocs, unless warn_missing_docs is passed. So the testcase should pass in rc2.

Choose a reason for hiding this comment

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

With erlang/otp#8192 the test should pass using both edoc and EEP-59 docs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for this, @garazdawi. I'll check it (your branch) and keep this open, after which maybe we can simply rebase (once your pr gets merged - come to think of it our CI should be targeting the main branch, not a specific tag).

@paulo-ferraz-oliveira
Copy link
Collaborator Author

paulo-ferraz-oliveira commented Feb 28, 2024

I've updated the tests to run, when post-27, all the existing tests plus the same tests with a 27+ variation.

(tests are expected to fail, when using OTP from master, until @garazdawi's branch gets merged, and is used by Bob - that which feeds setup-beam)

@paulo-ferraz-oliveira
Copy link
Collaborator Author

@garazdawi's branch was merged. I'm making CI go again.

@paulo-ferraz-oliveira
Copy link
Collaborator Author

All ✅ Good stuff, Lukas.

@starbelly
Copy link
Owner

@paulo-ferraz-oliveira Unfortunately, there are conflicts to be resolved now

@paulo-ferraz-oliveira
Copy link
Collaborator Author

No prob. I can get on those.

The strategy considered is:
1. ask to recreate the _checkouts/rebar3_ex_doc folder
   1.1. this should be done for, e.g., OTP 24 to start
2. don't recreate (further) the _checkouts/rebar3_ex_doc folder
   2.1. continue generation with e.g. OTP 25
3. conditionally download ex_doc from GitHub based on current
   OTP version (e.g. kerl/asdf-activated)

Everything else is pretty much the same, but this commit
also adds a minor change:
- priv/ex_doc_... isn't deleted (it's already ignored and
  is not required to be deleted - maybe a remnant from a
  past iteration?)
We choose based on prior downloaded elements
and the fact we know what we're running with
    erlang:system_info(otp_release).
"unknown" became a default analysis
I want to concentrate on what was working
_before_ 27.0-rc1 came out
It doesn't prevent anything from breaking, but it's
"more correct"
We increase our chances of finding an ex_doc we can use for
a specific OTP
We limit concurrency so that each "job" fetches the existing
cache to add to it
We want to make sure a job uses prior cache in the same
CI run
Erlang/Elixir/rebar3

This should give us a much more bleeding edge approach, though
it's possible we're increasing the chance of issues

Up to us to decide, later, if we wanna separate this step
into its own workflow which should not be required green
for merge (but kinda seen as a "control" flow)
In pre-27 we assume only the tests that existed;
in post-27 we assume all tests that existed plus the same tests
with new input (as post-27 -ready module)
@paulo-ferraz-oliveira
Copy link
Collaborator Author

I've rebased, and we should now get all ✅

@paulo-ferraz-oliveira
Copy link
Collaborator Author

I think we should be good to go. @starbelly, this might warrant a release, if just for testing with OTP 27 rc without workarounds.

@starbelly
Copy link
Owner

LGTM!

@starbelly starbelly merged commit b24093a into starbelly:main Mar 7, 2024
12 checks passed
@paulo-ferraz-oliveira paulo-ferraz-oliveira deleted the feature/broader-otp-support branch March 7, 2024 21:26
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.

Bundle more versions of ex_doc and choose per running OTP version
3 participants