canonical / sphinx-docs-starter-pack

A documentation starter-pack
https://canonical-starter-pack.readthedocs-hosted.com/
Other
15 stars 38 forks source link

Should Makefile targets be more specific? #101

Closed ru-fu closed 3 months ago

ru-fu commented 1 year ago

We use very general targets in the Makefile, like make install or make spelling. This makes it harder to integrate our Makefile with existing Makefiles - install would probably already exist in the development/code Makefile, and spelling would be misleading since it only checks the spelling in the documentation, not in code comments.

Should we rename the targets to, for example, doc-install and doc-spelling?

This would be a breaking change though (unless we include aliases, which should be easy to remove).

An additional reason to change is that we have the https://github.com/canonical/documentation-workflows repository that requires specific targets - so if I cannot include an install target (which installs only the documentation tools) in the Makefile for my project, I cannot use the reusable workflow.

syncronize-issues-to-jira[bot] commented 1 year ago

Thank you for reporting us your feedback!

The internal ticket has been created: https://warthogs.atlassian.net/browse/DOCPR-111.

This message was autogenerated

pmatulis commented 1 year ago

This is a good idea to me. 👍🏻

akcano commented 5 months ago

@ru-fu Getting back to this after a round of soul-searching.

I've added a PoC in my fork; please take a look and let me know if it works as expected: https://github.com/akcano/sphinx-docs-starter-pack/tree/MAKEFILE

However, I would strongly consider simply renaming the SP Makefile and updating the workflows instead; this seems a better, cleaner approach to me.

ru-fu commented 5 months ago

However, I would strongly consider simply renaming the SP Makefile and updating the workflows instead; this seems a better, cleaner approach to me.

Can you explain a bit more what you mean here?

Your approach looks like it'll work fine to allow overriding names to avoid clashes with other Makefiles, but I don't see how it'll fix the problem of the shared workflows requiring specific targets. If my product already has an install target, the shared workflow will call this one instead of the sp-install target. And if we update the shared workflow to require the sp-install target instead, it won't be found unless we also specify the Makefile name, which will be a breaking change again ...

So to deal with this problem, we'll need to fix the shared workflows to allow overriding the target name I think. Maybe that's what you meant above?

akcano commented 5 months ago

@ru-fu In brief, yes; let our doc-specific workflows invoke the doc-specific Makefile, and leave the rest aside.

However, I have a question: how often have we seen such clashes in the wild? I assume we have two major use cases for the starter pack; correct me, please, if there's anything else.

  1. Monorepo: the starter pack is cloned under docs or some other subdirectory of the main repo, but this is easily overcome:
jobs:
  documentation-checks:
    uses: canonical/documentation-workflows/.github/workflows/documentation-checks.yaml@main
    with:
      working-directory: './docs/'
  1. Separate doc-oriented repo: a snapshot/fork of the starter pack, essentially. Do we expect any more Makefiles to occur there?
ru-fu commented 5 months ago

The third scenario is that the starter pack Makefile isn't used, and its targets are integrated/duplicated/customised in the main Makefile of the project.

Not sure if that is a common use case (and if we should actually support it), but this is what we currently have in LXD. The main reason for that is that this is how we did it before the starter pack work started - but we also have some custom steps required for the doc build that require targets from the main Makefile. There are probably ways to call them from a Makefile in the docs repo, but I haven't tested that so far.

akcano commented 5 months ago

Understood, thanks. In this case, the way I see a solution is this:

  1. Singling out SP-specific targets into a custom Makefile (sp-install, etc.).
  2. Running them under non-sp aliases from the default Makefile in the starter pack (install etc.).
  3. Switching the automated workflows to use the custom Makefile and sp-prefixed targets.

This way, your case can be resolved by dropping the default Makefile entirely, and the other two won't be affected; the workflows stayc operational, and the only visible change is that local runs will require the sp-prefixed targets (moved to the default Makefile or left under a custom one).

Let me know what you think.

ru-fu commented 5 months ago

Yes, that sounds good to me!

The only issue I see is with

Switching the automated workflows to use the custom Makefile and sp-prefixed targets.

This will mean that the workflows will break for every team that hasn't updated to the latest version of the starter pack - and this is something I would really like to avoid right now (because everyone will need to update when we move to the extension, and this change isn't pressing enough to make everyone update again before that).

Maybe we can simply add a new shared workflow that uses the targets from the sp-Makefile, and use that one going forward (but leave the existing one as it is)?

akcano commented 5 months ago

I think it's the best option we have.

akcano commented 5 months ago

@ru-fu : to clarify, I'm going to update this PR a bit and add a second one to @canonical/documentation-workflows to introduce the new workflows. After that, it's a matter of personal maturity for everyone to finish the transition :)

akcano commented 5 months ago

@ru-fu : canonical/documentation-workflows#13

akcano commented 4 months ago

@ru-fu I'll test this PR with the updated workflows and let you know if it's good to go.

akcano commented 4 months ago

@ru-fu I published the update as a PR, seems it runs fine.