erlef / rebar3_hex

Rebar3 Hex library
Apache License 2.0
101 stars 49 forks source link

Publishing Meck 0.9.1 docs silently fail #205

Closed eproxus closed 3 years ago

eproxus commented 3 years ago

(cc @ericmj)

When publishing the docs for Meck 0.9.1 the task indicates success but the docs are not published correctly:

> DEBUG=1 rebar3 hex docs
===> Load global config file /Users/user/.config/rebar3/rebar.config
===> 23.2.6 satisfies the requirement for minimum OTP version 18
===> Evaluating config script "/Users/user/.cache/rebar3/plugins/hex_core/rebar.config.script"
===> Compile (apps)
===> Expanded command sequence to be run: []
===> Running provider: do
===> Expanded command sequence to be run: [app_discovery,install_deps,lock,{hex,docs}]
===> Running provider: app_discovery
===> Found top-level apps: [meck]
    using config: [{src_dirs,["src"]},{lib_dirs,["apps/*","lib/*","."]}]
===> Running provider: install_deps
===> Verifying dependencies...
===> Running provider: lock
===> Running provider: {hex,docs}
Local Password:                         
===> Published docs for meck 0.9.1 
>

However, the link on Hex.pm is broken:

Environment

> rebar3 --version
rebar 3.14.4 on Erlang/OTP 23 Erts 11.1.8
> rebar3 as global plugins list
--- Global plugins ---
rebar3_hex (6.10.3)
ericmj commented 3 years ago

I was CCed but I am not sure what the question/issue is. Both links work for me, are there any specific URLs you have issue with?

eproxus commented 3 years ago

Ok, page just works now, cache issue perhaps? Sorry for the noise 😕

ericmj commented 3 years ago

You may need to clear your local browser cache if you have visited the page before.

eproxus commented 3 years ago

Some background to why I was thinking this was an actual issue.

I vaguely remember that when publishing the new package version itself (via rebar3 hex publish) it also said docs were published (@starbelly can you confirm? I might be entirely wrong here). That’s why I thought they should have already been published, and when trying publish them again and hitting my old Page not found cache I thought it “failed again”.

Again, sorry for the misreport.

eproxus commented 3 years ago

Just released a new version:

===> Verifying dependencies...
===> meck.app.src : deprecated field maintainers found
Publishing meck 0.9.2 to hexpm
  Description: A mocking framework for Erlang
  Dependencies:

  Included files:
    CHANGELOG.md
    LICENSE
    NOTICE
    README.md
    rebar.config
    rebar.lock
    src/meck.app.src
    src/meck.erl
    src/meck.hrl
    src/meck_args_matcher.erl
    src/meck_code.erl
    src/meck_code_gen.erl
    src/meck_cover.erl
    src/meck_expect.erl
    src/meck_history.erl
    src/meck_matcher.erl
    src/meck_proc.erl
    src/meck_ret_spec.erl
    src/meck_util.erl
  Licenses: Apache 2.0
  Links:
    Changelog: https://github.com/eproxus/meck/blob/master/CHANGELOG.md
    GitHub: https://github.com/eproxus/meck
  Build tools: rebar3
Be aware, you are publishing to the public Hexpm repository.
Before publishing, please read Hex CoC: https://hex.pm/policies/codeofconduct
Proceed? ("Y")> Y
Local Password:
===> Published meck 0.9.2
===> Published docs for meck 0.9.2

The last line indicated docs would be published, but I'm still getting the "Page not found" error. Is there some time delay before docs are actually published/renderd or is it a bug in the rebar3 hex publish task that it doesn't actually publish the docs?

paulo-ferraz-oliveira commented 3 years ago

Hm... that's kinda odd, yeah. Same thing happens to me - and it's not a cache issue - (though I've published in the past without seeing it, too).

starbelly commented 3 years ago

Yeah, I see page not found too. I assume this is an issue on hex.pm side. Docs usually show up immediately, but there can be a delay, but I would not expect a 4 hour delay. I had recently tried testing publishing docs to a local hexpm instance and did not find any issues FWIW. However, I can double check as soon as I have a minute just to be on the safe side.

@eproxus I assume you ran edoc before rebar3 hex publish?

ericmj commented 3 years ago

Even if the docs are published does not mean they are correct, you can publish an empty tarball of docs and get the "Published docs for meck 0.9.2 " message. Please build the docs locally and verify that they are correct and include a index.html file. index.html is the default page so if it's not included then a 404 error will be shown.

eproxus commented 3 years ago

Then I would consider it a bug in the rebar3 hex publish task that it doesn't run the edoc task first. I think this message happens because I have an EDoc input file in the docs folder which is probably shipped to Hex.pm. Either that or the task should not automatically publish docs if there is no docs/index.html. Even so, it should probably run the edoc task anyway because who knows how stale any existing index.hmtl file is. Otherwise you could accidentaly publish old docs for a new version.

@ericmj Is there any limit on how long after the initial publish you are allowed to publish docs?

ericmj commented 3 years ago

Is there any limit on how long after the initial publish you are allowed to publish docs?

No, you can republish docs at any time, but make sure you clear your browser cache when verifying the publish.

starbelly commented 3 years ago

@eproxus The behavior was changed for publishing, by default we will check to see if you already have a doc directory, if you do we will publish it, and yes if it's empty, that could be published, so I agree it's a bug. I'm about to merge in a change that will allow you to specify the default doc provider in config (namely edoc) which will resolve this issue (i.e., if edoc provider is configured, we will automatically run edoc, etc.)

paulo-ferraz-oliveira commented 3 years ago

I would consider it a bug in the rebar3 hex publish task that it doesn't run the edoc task first

@eproxus, I think @starbelly started some work around doc providers, a while back.

I would not assume that edoc is the only doc publisher, but agree that if index.html is not present the "docs were published" message should not be issued.

ferd commented 3 years ago

Running edoc by default was removed specifically for other languages on the platform that have other docs providers iirc.

starbelly commented 3 years ago

@eproxus 6.11.0 was just released and you can now add configuration as follows to auto-generate docs for publishing.

%%   - `{doc, Options}' as part of your global hex config, where `Options' is a map.
%%   - `#{doc => Options}' as part of a specific repo configuration, where `Options' is a map
%%
%%  Repo specific config will always override global hex config if the repo in question is
%%  the context in which rebar3_hex is operating in.
%%
%%  Supported options:
%%
%%  - `provider' - This value of this option should be the name of a valid doc
%%                 provider, such as `edoc'. Note that only `edoc' is supported out of
%%                 the box with rebar3. Refer to `src/rebar_prv_edoc.erl' as an example
%%                 of a docs provider in `rebar3', as well as
%%                 https://rebar3.org/docs/tutorials/building_plugins/ for documentation on
%%                 creating plugins.
%%
%%  Example global config within rebar.config :
%%
%%  `{hex, {doc, #{provider => edoc}}}.'
%%
%%  Example repo specific config:
%%  ```
%%  {hex, [
%%        {repos, [
%%                 #{name => <<"my_private_hex">>,
%%                   repo_url => <<"https://my_private_hex.foo">>,
%%                   doc => #{provider => edoc}
%%                  }
%%                ]
%%         }
%%       ]
%%   }.

We're going to add some other enhancments to both the publish task and docs task such as --without-docs.

Anyway, I think this resolves the issue, but won't close until I hear back.

Edit:

https://github.com/tsloughter/rebar3_hex/pull/210 😁

Edit:

https://github.com/tsloughter/rebar3_hex/pull/211 😃

Edit:

https://github.com/tsloughter/rebar3_hex/pull/212

Edit:

@eproxus And cut 6.11.1 which includes pr #210 and #211, we'll cut another patch release after #212 is merged.

eproxus commented 3 years ago

Great improvements! 👍

If the publish task still publishes possibly random out of date things in the doc directory, I would still consider this a regression and not fixed. Even if it checks for index.html first.

If there is configuration for auto-generating docs on publishing, I don’t see why the task publishes anything by default. That provides little value to anyone that I can think of and just leads to mistakes.

starbelly commented 3 years ago

@eproxus I opened up an issue for that #213. I do not consider it a regression per it being the behavior some wanted, but I don't like it all. Thus the issue. I do think publishing docs by default if they are configured makes sense though, I think this is generally expected, and is inline with mix hex, which is one of our goals (i.e., alignment with mix hex).

Edit:

I think what we can do is further align with mix hex and provide a package task to go along with our docs task. I was originally going to do these as options on publish and perhaps that might have been the best way to go (--package and --docs). Regardless, I do think publish by default should handle publishing the package and docs, unless you tell it not to (if a doc provider is configured).

eproxus commented 3 years ago

As far as I remember docs weren’t automatically published at all before in earlier versions? (not exactly sure which)

starbelly commented 3 years ago

@eproxus No, it was not, and I'd have to dig into see exactly what version this was introduced, but sometime after version 6.2. As stated, @tsloughter and I along with others agreed aligning with mix hex is an important goal for us.

Edit: One thing we could do, which I'm definitely not sure about, is making the publishing of docs automagically something you can opt out of in config, but I'd favor just adding a package task to avoid bringing in more options and configuration. As long as it's documented how to only publish a package or only publish docs, I don't see why config like this would be needed though.

eproxus commented 3 years ago

I agree about the outlined plan in #213. If I understood it correctly the --without-docs options would be removed and change to --with-docs or something similar?

starbelly commented 3 years ago

@eproxus #213 well, I'm not sure about removing --without-docs, that's specifically for the case where you do not want to publish docs on the publish task. This is about implicitly publishing if doc dir directory exists, which I think we both agree is not good. Specifically for the case where someone does not use edoc. So we could add an option on both the publish and docs test that is --with-docs or --doc-path or something similar and ditch publishing docs UNLESS you have edoc configured as a provider or you use --without-docs (and have edoc configured) option. Does that make sense?

eproxus commented 3 years ago

@starbelly Ah, got it. The option is useful to manually disable automatic doc generation and publishing (if configured). I don't think I'll need the --with-docs options if I can configure the doc publishing in rebar.config.

starbelly commented 3 years ago

@eproxus Specifically, are you asking for an option in config to say "never automatically generate and publish docs"?

Edit: On the publish task specifically, obviously if you're running the docs task you expect it to publish (if configured)

eproxus commented 3 years ago

@starbelly I’m just getting more and more confused here. To summarize my wish implementation:

  1. Docs are automatically published with the package itself if configured.
  2. The existing doc folder content is never published unless explicitly configured.
  3. Docs could be separately published if desired (I would personally only do this for corrections)
  4. There should be a flag to the package publish task to skip publishing docs if so desired.
  5. It should be possible to configure different kinds of doc providers (such as EDoc, static files, etc.)

Have I missed anything?

The regression and bug that exists currently breaks (2).

ferd commented 3 years ago

I think step 2 is what we were looking to change iirc.

The problem statement goes a bit like "people going on hex looking for packages expect to see documentation, and it turns out all the Elixir libraries have it and none of the Erlang libraries do and it looks sort of bad" -- after asking around it turns out a few people do have documentation, but they had no idea they could even publish it along packages, so the attempt was to push doc if it exists, and warn if it can't be generated (but still publish the package) as a way to nudge people in that direction and make them aware of the feature without otherwise interrupting their flow too much.

eproxus commented 3 years ago

Agree, it is a great goal. As it stands right now it is actually worse in my opinion, since it publishes outdated content in the doc folder by default. In the case of Meck it published unusable files leading to docs actually missing (where I used to manually publish them before). If I would have generated docs for an earlier version it would have published those for a newer version.

Wrong docs are worse than no docs.

EDIT: realizing I might be preaching to the choir here. 😄

starbelly commented 3 years ago

@ferd Can you add your thoughts on #213. A simple yes or no will suffice.