InsightSoftwareConsortium / ITK

Insight Toolkit (ITK) -- Official Repository. ITK builds on a proven, spatially-oriented architecture for processing, segmentation, and registration of scientific images in two, three, or more dimensions.
https://itk.org
Apache License 2.0
1.47k stars 686 forks source link

Superfluous file `build_text.js.in` for documentation generation. #5190

Closed albert-github closed 1 week ago

albert-github commented 2 weeks ago

Description

In the repository a file buid_text.js.in exists that is probably superfluous. Content:

$(function(){
    document.getElementById("datetime").textContent = "@_DATETIME@"
});

Expected information

Not using a locally defined date time function for the documentation but using in the header / footer $datetime and in body text use the doxygen command \showdate (available since doxygen version 1.9.5). When the file is necessary it would be better not to place it directly in the html output directory, but in another directory in the build tree and use the setting HTML_EXTRA_FILES to copy the file to the destination folder.

Actual information

No datetime found in the repository (except for build_text.js.in)

Versions

master acaddf4e78

Additional Information

Saw a problem when I ran some tests and removed the html directory and afterwards ran doxygen outside of cmake / nmake (which is not the best way but quite convenient during debugging.

albert-github commented 2 weeks ago

@blowekamp Nice to give a thumb down but it would be better to give a reason for it as well!

blowekamp commented 2 weeks ago

@albert-github I have been instructed that I can publish code but not engage with the public at this time. Thank you for your understanding.

albert-github commented 2 weeks ago

Well I hope that someone else will have the permission to engage with the public and can explain a thumb down.

dzenanz commented 2 weeks ago

I was also puzzled by the thumbs down without explanation. I guess it means that buid_text.js.in is not superfluous.

dzenanz commented 2 weeks ago

Maybe it is related to this? an “immediate pause” had been ordered on — among other things — regulations, guidance, announcements, press releases, social media posts and website posts until such communications had been approved by a political appointee.

albert-github commented 2 weeks ago

@dzenanz Looks like it, but as long as @blowekamp cannot share the reason for his thumbs down we will be riding in the dark. Maybe @blowekamp has the possibility to communicate it directly to you.

dzenanz commented 2 weeks ago

No, he sent me an email with the same statement (I have been instructed...).

dzenanz commented 2 weeks ago

Let's keep this issue open until NIH lifts this (hopefully transitional and short-lived) policy.

albert-github commented 2 weeks ago

A very very big pity and I hope it will soon be possible that the NIH lifts this policy.

thewtex commented 2 weeks ago

Hi @albert-github ,

I think the reason @blowekamp added this file is still valid. It is explained in the associated commit message:

The date and time are not set by javascript when the document is loaded. This removes nightly modification of all the HTML files, and enable traditional static HTML cache to work, and enable the generated HTML to be committed to git regularly without nightly modification.

albert-github commented 2 weeks ago

@thewtex Thanks. Might be the case but so in other words even when the file didn't change but has been regenerated from new source and the file didn't change (in the doxygen sense) it still is changed as the value of datetime is changed.

Committing result files like HTML files to git is in my opinion bad practice (they can be as artifacts with releases in the zip / tar.gz files which should be sufficient).

I still don't see the need for it, and as written before when one wants to have a unique date in the HTML file there is still the doxygen $datetime that can be placed in e.g. the header or footer file and will put the date / time of generation in the file.

Furthermore there might be numerous other files as well, like svg, js, ... so these won't get a timestamp either.

I think it is necessary to know exactly why this js code should be present, so probably we have to wait till @blowekamp can engage with the public again.

thewtex commented 2 weeks ago

@albert-github more context:

We used GitHub Pages to publish the HTML. It used to be necessary to commit to Git for GitHub Pages. Recently, GitHub provided an alternative for GitHub Pages that does not require Git commits. We did move to that recently, along with publishing to ReadTheDocs (see the ITK 5.4.0 release notes for more information on that).

However, there is still the issue of browser cache hits. With the ReadTheDocs CDN, the responsiveness of loading the documentation is greatly improved. However, we may want to keep this. I agree that we should get @blowekamp 's input before making changes.

albert-github commented 2 weeks ago

FYI

blowekamp commented 1 week ago

However, there is still the issue of browser cache hits. With the ReadTheDocs CDN, the responsiveness of loading the documentation is greatly improved. However, we may want to keep this. I agree that we should get @blowekamp 's input before making changes.

Thank you for your understanding in my delayed response.

This change was added to minimize changes of daily updates of the documentation. Yes, it was for git commit based gh-pages along with browser caches. The deployment of the Doxygen is certainly evolving with improve GH actions and read the docs hosting. Importance of this is likely less than it was.

  • the cache can indeed be quite usable but also quite a nuisance, though with $datetime this should also not happen (has similar functionality as the javascript

If this achieve the same goal, without the custom code that is good!

dzenanz commented 1 week ago

@albert-github can you propose a PR or a diff?

albert-github commented 1 week ago

Tomorrow I'll have a go at it .

albert-github commented 1 week ago

I've looked a bit deeper into the problem her and it has to do with the the file: Documentation/Doxygen/DoxygenFooter.html with contents:

<!-- HTML footer for doxygen 1.8.15-->
<!-- start footer part -->
<!--BEGIN GENERATE_TREEVIEW-->
<div id="nav-path" class="navpath"><!-- id is needed for treeview function! -->
  <ul>
    $navpath
    <li class="footer">$generatedby
    <a href="https://www.doxygen.org/index.html">
    <img class="footer" src="$relpath^doxygen.png" alt="doxygen"/></a> $doxygenversion </li>
  </ul>
</div>
<!--END GENERATE_TREEVIEW-->
<!--BEGIN !GENERATE_TREEVIEW-->
<hr class="footer"/>
<div class="footer" align="left">
  <small>Tarballs of release and nightly generated Doxygen documentation
    are available in the
    <a href="https://github.com/InsightSoftwareConsortium/ITKDoxygen/releases">
      <i>InsightSoftwareConsortium/ITKDoxygen</i> GitHub Releases
    </a>.
  </small>
</div>
<address class="footer"><small>
Generated on <span id="datetime">unknown</span> for $projectname by &#160;
<a href="https://www.doxygen.org/index.html">
<img class="footer" src="$relpath^doxygen.png" alt="doxygen"/>
</a> $doxygenversion
</small></address>
<!--END !GENERATE_TREEVIEW-->
</body>
</html>

and in particular the line:

Generated on <span id="datetime">unknown</span> for $projectname by &#160;

which is used in case of GENERATE_TREEVIEW=NO and resulting in something like (from: https://docs.itk.org/projects/doxygen/en/stable/index.html):

Image

Now looking at the reasoning in the commit introducing the build_text.js.in:

Date: Wednesday, December 29, 2021 6:10:12 PM
Committer: Hans Johnson
Commit Date: Tuesday, January 4, 2022 6:32:46 PM
ENH: remove rendering datetime with Doxygen into HTML footer

The date and time are not set by javascript when the document is
loaded. This removes nightly modification of all the HTML files, and
enable traditional static HTML cache to work, and enable the generated
HTML to be committed to git regularly without nightly modification.

So when a page doesn't change it should not be re-uploaded by means of a nightly build but during the display of the page the right build time should be shown.

My proposal was to use $datetime but at that moment doxygen would replace the $datetime by the build time and thus all the html pages would change and would cause a huge upload.

Conclusion

Side note: I do like the construct, so I'm seriously thinking about creating a proposed pull request for doxygen based on the experience from this issue.

dzenanz commented 1 week ago

Should this issue be closed? Or kept open awaiting the implementation of this feature in doxygen?

albert-github commented 1 week ago

I think best is to close it, implementation in doxygen will, when accepted, earliest be in 1.14.0