Clean old_artifact_path By Default

[ghost]

As part of 8180356 a clean method was added to the Sphinx build to cleanup the checkout directory before beginning a build.

The clean method is in doc_builder/base.py file. However, it is manually called only in the Sphinx builder. Not the Mkdocs builder.

Could the clean method be called by default as part of the finish_build task in tasks.py to delete the old_artifact_path directory?

This should have no impact on the rest of the checkout directory contents.

[ericholscher]

This seems like a good idea. The main issue is that Sphinx caches things in the build directories, but as long as we're removing the outputed _build/html directory, and not the _build/doctrees, this shouldn't effect the build caches.

[d0ugal]

MkDocs already cleans up. So this probably wont add much, but wont cause harm either.
See: 54130c3

[ghost]

Nice catch @d0ugal. Sorry I missed that in the first place.

Maybe we can do away with clean() from base.py and just require that each builder be responsible for the level of cleanup necessary before building?

We can just move clean() into sphinx.py and keep the call in there.

[d0ugal]

I think it is still a reasonable think to do in the base builder - it will make sure things are consistent if a new builder is ever added.

[ghost]

@d0ugal so rather than calling it in finish_build, somehow call it in base.py?

What are your thoughts on strategies for calling it when build is called? My only thought is to create a decorator that can be applied to the build method of each builder, and the decorator would call clean.

However, that stills requires every developer of a builder to remember to add the decorator.

Is that reasonable?

[d0ugal]

I'm not sure :) I am not familiar enough with the RTD code to say.

[gregmuellegger]

@destroyerofbuilds (your name is quite funnily related to this ticket, is this intended? 😄)

As you mentioned adding a decorator wouldn't be good as it's not enforced then for every builder. I would consider adding a pre_build method in the base builder where the call of self.clean should live. Then we call pre_build everytime before we call build.

What do you think?

[ghost]

@gregmuellegger the pre_build method is a possibility, but that would, to me, seem like a trade off, we don't have to worry about forgetting to add the decorator to every builder, but now we have to remember to call pre_build everywhere we call build.

For a brief moment I started looking at creating a BaseBuilder metaclass that would wrap the build method in a function that would cleanup the docs directory, and then call the build method of the newly instantiated instance of the particular builder.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[stsewd]

This is still valid for consideration.

[humitos]

Our Sphinx builder is already cleaning the old_artifact_path and MkDocs is cleaning it by using the --clean option when called. I don't see too much value here than refactoring the Base builder class to force calling the .clean() method in all the builders and rely on their logic for this.

However, we don't have more builders at this point and we are not planning to add another one for now. So, I'm closing this issue here. We can revisit if needed.

Related with the doctree directory for Sphinx: #4363