feat: tagfiles generation for cross-referencing in doxygen format

[fpelliccioni]

Fixes #650

[alandefreitas]

The logic for the Writer looks great. I think the only issue is where it would be called from.

[alandefreitas]

The problem CI has been fixed. Could you rebase this?

[alandefreitas]

However, we will still have a problem with the USR from Krystian's commit blocking CI.

I want to start by comparing the XML in the artifacts. Do you have the result for Boost.URL multipage? That's the one we can't risk breaking because it's already in production. It has to be compatible with the existing one.

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

It fails with

assertion failed: n <= indent_.size() on line 203 in src/lib/Gen/xml/XMLTags.cpp

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[alandefreitas]

The implementation of the tagfile write is excellent.

CI failed after I rebased. That was a good thing because it helped me catch a bug.

So, the reason CI is failing is trivial. The TestRunner calls if(auto err = gen_->buildOneString(generatedDocs, **corpus)), which calls Generator::buildOneString, where you changed the call to buildOne to buildOne(ss, corpus, "");. We could fix CI by just adapting AdocGenerator::buildOne to not generate the tagfile when this string is empty. However, this doesn't address the deeper issue here.

The real issue causing all of that is the API design. Your new fileName parameter in Generator::buildOne is breaking the contract of this abstract class.

1. First of all, the new parameter is documented as @param fileName The file name to use for the output., but that's not what the implementation is doing. The filename is the filename for the tagfile, which is unrelated to the filename of the output. The parameter isn't even used in the XMLGenerator. It's even more confusing because fileName is not the filename of the tagfile but the basename to which "tag.xml" will be appended to form the tagfile filename.
2. Most importantly, the contract of Generator::buildOne is that it renders the documentation to any output stream regardless of the filename because (i) there's not even a filename for most output types and (ii) if there were a file for each stream, the stream would become redundant. For instance, tests use buildOneString, which outputs the result to a string.

This bug was already there, but CI only made it evident. It worked before rebasing because Generator::buildOneString wasn't being covered with the AdocGenerator.

Because of how Generator expects the output to be decoupled from any filename and buildOne is based on streaming only that result to the output, the Generator should have received new virtual functions to buildTagfile(std::ostream& os, Corpus const& corpus) or something instead of buildOne trying to fill this role.

If this is hard to achieve because we can't decouple the logic of buildOne and buildTagfile, a second-best would be buildOne(std::ostream& os, Corpus const& corpus, std::ostream& tagFileOs) instead of buildOne(std::ostream& os, Corpus const& corpus, std::string_view fileName) and a new overload in Generator without std::ostream& tagFileOs which would call buildOne with a noop std::ostream for the tagFileOs.

This fixes the underlying problem and the original issue with buildOneString that's breaking CI.

[fpelliccioni]

The implementation of the tagfile write is excellent.

CI failed after I rebased. That was a good thing because it helped me catch a bug.

So, the reason CI is failing is trivial. The TestRunner calls if(auto err = gen_->buildOneString(generatedDocs, **corpus)), which calls Generator::buildOneString, where you changed the call to buildOne to buildOne(ss, corpus, "");. We could fix CI by just adapting AdocGenerator::buildOne to not generate the tagfile when this string is empty. However, this doesn't address the deeper issue here.

The real issue causing all of that is the API design. Your new fileName parameter in Generator::buildOne is breaking the contract of this abstract class.

1. First of all, the new parameter is documented as @param fileName The file name to use for the output., but that's not what the implementation is doing. The filename is the filename for the tagfile, which is unrelated to the filename of the output. The parameter isn't even used in the XMLGenerator. It's even more confusing because fileName is not the filename of the tagfile but the basename to which "tag.xml" will be appended to form the tagfile filename.
2. Most importantly, the contract of Generator::buildOne is that it renders the documentation to any output stream regardless of the filename because (i) there's not even a filename for most output types and (ii) if there were a file for each stream, the stream would become redundant. For instance, tests use buildOneString, which outputs the result to a string.

This bug was already there, but CI only made it evident. It worked before rebasing because Generator::buildOneString wasn't being covered with the AdocGenerator.

Because of how Generator expects the output to be decoupled from any filename and buildOne is based on streaming only that result to the output, the Generator should have received new virtual functions to buildTagfile(std::ostream& os, Corpus const& corpus) or something instead of buildOne trying to fill this role.

If this is hard to achieve because we can't decouple the logic of buildOne and buildTagfile, a second-best would be buildOne(std::ostream& os, Corpus const& corpus, std::ostream& tagFileOs) instead of buildOne(std::ostream& os, Corpus const& corpus, std::string_view fileName) and a new overload in Generator without std::ostream& tagFileOs which would call buildOne with a noop std::ostream for the tagFileOs.

This fixes the underlying problem and the original issue with buildOneString that's breaking CI.

This issue is addressed. I’ll let you know once CI passes so you can review, and if it’s good to go, feel free to merge.

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[fpelliccioni]

I downloaded the demo artifacts and saw no tag files in there.

@alandefreitas

You need to specify the tagfile-path option. By default, it's empty, and no tag file will be generated unless explicitly set.

Currently, tagfile-path is defined as "relativeto": "<config-dir>" in the configuration.
I'm wondering if this is the best approach or if it should require an absolute path instead.

[alandefreitas]

By default, it's empty

We can change that default. Or at least enable it in CI. This can't go untested. We need these file artifacts in CI.

if it should require an absolute path instead

We never require an absolute path. The tagfile is relative to the output path. Not the config path.

[fpelliccioni]

By default, it's empty

We can change that default. Or at least enable it in CI. This can't go untested. We need these file artifacts in CI.

if it should require an absolute path instead

We never require an absolute path. The tagfile is relative to the output path. Not the config path.

I didn’t see a way to make the tagfile path relative to the output path directly. I believe I’ll need to update the code handling the configuration options to support this behavior (though I’m not entirely sure yet).

I’ll go ahead and make the necessary changes, but I’ll address this in a separate PR.

[alandefreitas]

Whatever you put in "relativeto" will go to an OptionProperties::relativeto string in the public settings. This function is called for all arguments so you can normalize them:

https://github.com/cppalliance/mrdocs/blob/develop/src/lib/Lib/Config.cpp#L75

When the argument is a relative path, it gets the base path for normalization here:

https://github.com/cppalliance/mrdocs/blob/develop/src/lib/Lib/Config.cpp#L95

The base path can come from "relativeto" or from the default path prefix. There's no default prefix because the string is empty by default. When it comes from "relative-to", we get it with this function call:

mrdocs/src/lib/Lib/Config.cpp

Line 229 in 6156756

return getBaseDir(relativeTo, dirs);

The function only considers the Reference directories for now:

mrdocs/src/lib/Lib/Config.cpp

Line 192 in 6156756

if (referenceDirKey == "config-dir") {

It doesn't currently consider other options as valid prefixes. It would probably need to change to consider the PublicSettings& self.

One thing to consider is that the output path will need to be normalized before the tagfiles path. In practice, it means the output needs to come before the tagfile option in the json file. In principle, the python script could identify dependencies between options to rearrange them but I don't think it's something we need for now.

[fpelliccioni]

Whatever you put in "relativeto" will go to an OptionProperties::relativeto string in the public settings. This function is called for all arguments so you can normalize them:

https://github.com/cppalliance/mrdocs/blob/develop/src/lib/Lib/Config.cpp#L75

When the argument is a relative path, it gets the base path for normalization here:

https://github.com/cppalliance/mrdocs/blob/develop/src/lib/Lib/Config.cpp#L95

The base path can come from "relativeto" or from the default path prefix. There's no default prefix because the string is empty by default. When it comes from "relative-to", we get it with this function call:

mrdocs/src/lib/Lib/Config.cpp

Line 229 in 6156756

return getBaseDir(relativeTo, dirs);

The function only considers the Reference directories for now:

mrdocs/src/lib/Lib/Config.cpp

Line 192 in 6156756

if (referenceDirKey == "config-dir") {

It doesn't currently consider other options as valid prefixes. It would probably need to change to consider the PublicSettings& self.

One thing to consider is that the output path will need to be normalized before the tagfiles path. In practice, it means the output needs to come before the tagfile option in the json file. In principle, the python script could identify dependencies between options to rearrange them but I don't think it's something we need for now.

PR created #743

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html

[cppalliance-bot]

An automated preview of the documentation is available at https://670.mrdocs.prtest2.cppalliance.org/index.html