Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Oversized ITK logo in Doxygen documentation #4785

Closed
jhlegarreta opened this issue Jul 20, 2024 · 19 comments
Closed

Oversized ITK logo in Doxygen documentation #4785

jhlegarreta opened this issue Jul 20, 2024 · 19 comments
Labels
Good first issue A good issue for community members new to contributing type:Documentation Documentation improvement or change

Comments

@jhlegarreta
Copy link
Member

Description

The ITK logo is shown oversized in the Doxygen documentation:
https://itk.org/Doxygen/html/

Expected information

The ITK logo should have a reasonable size in the Doxygen documentation welcome page.

Actual information

The ITK logo has an excessively large size.

Versions

master.

Additional Information

Visited in Chrome Version 126.0.6478.126 (Official Build) (64-bit).

@jhlegarreta jhlegarreta added type:Documentation Documentation improvement or change Good first issue A good issue for community members new to contributing labels Jul 20, 2024
@viv511
Copy link
Contributor

viv511 commented Jul 20, 2024

Hi! I would love to take this up as my first open source issue. I would appreciate it if I could work on this. :)

@jhlegarreta
Copy link
Member Author

@viv511 Feel free to propose a PR.

@N-Dekker
Copy link
Contributor

Any idea how the logo became so large? Just curious. It wasn't that large in the past! Anyway, I'm glad it's being addressed @jhlegarreta @viv511 👍

@thewtex
Copy link
Member

thewtex commented Jul 23, 2024

@viv511 we would welcome your help with this!

Logos of different sizes can be found here:

https://github.com/InsightSoftwareConsortium/ITK/tree/master/Documentation/Art

Any idea how the logo became so large?

It was updated recently :-)

@viv511
Copy link
Contributor

viv511 commented Jul 23, 2024

Hi! Thank you all so much!

Would you rather me change the image to a smaller image in the Art folder or scale down the "itkLogo.png" image down 50%?

I had previously thought that the code was written in HTML, but after looking around I believe it uses a Doxygen file to display the image. I believe the relevant code is here I looked at the image docs and I think I can append the optional size parameters to fix this.

As this is my first time contributing, I've tried my best to follow the guidelines of ITK contributing. When I tried to commit my change on a branch on the cloned repo, I got the following error message:

pre-commit hook failure
-----------------------

clang-format executable was not found.

A clang-format binary will be downloaded and configured when ITK
is built with the (i) BUILD_TESTING=True and (ii) ITK_USE_CLANG_FORMAT=True CMake configuration options enabled.

Alternatively, install clang-format version 8.0 or set the executable location with

  git config clangFormat.binary /path/to/clang-format

I'm sorry if the fix is intuitive, but I'm unable to figure out what I need to change without breaking any other parts of the code. I appreciate your patience and help a lot!

Furthermore, I also had troubles with locally testing the doxygen files. I tried turning on the CMAKE flags BUILD_DOCUMENTATION and ITK_BUILD_ALL_MODULES and also tried to edit the Doxyfile configuration, but neither of these methods gave me any success. It is my first time working with Doxygen in a larger repository like this, so I would really appreciate the help in understanding how to correctly locally test the doxygen changes so that I can be more effective in the future.

Thank you all so much again,
@viv511

@thewtex
Copy link
Member

thewtex commented Jul 23, 2024

Would you rather me change the image to a smaller image in the Art folder or scale down the "itkLogo.png" image down 50%?

@viv511 the image from the Art folder that is used could be changed or a different file with a different size could be created. But we could leave itkLogo.png as-is in case it has been used in a different context.

I got the following error message:

pre-commit hook failure
-----------------------

clang-format executable was not found.

A clang-format binary will be downloaded and configured when ITK
is built with the (i) BUILD_TESTING=True and (ii) ITK_USE_CLANG_FORMAT=True CMake configuration options enabled.

Alternatively, install clang-format version 8.0 or set the executable location with

  git config clangFormat.binary /path/to/clang-format

I'm sorry if the fix is intuitive, but I'm unable to figure out what I need to change without breaking any other parts of the code. I appreciate your patience and help a lot!

If that message is not intuitive, you can skip the pre-commit hooks by passing -n to git commit for now. I assume you are on an ARM system? We are in the process of improving automated fetching and configuration of the clang-format binary.

Furthermore, I also had troubles with locally testing the doxygen files. I tried turning on the CMAKE flags BUILD_DOCUMENTATION and ITK_BUILD_ALL_MODULES and also tried to edit the Doxyfile configuration, but neither of these methods gave me any success. It is my first time working with Doxygen in a larger repository like this, so I would really appreciate the help in understanding how to correctly locally test the doxygen changes so that I can be more effective in the future.

The Dockerfile and build scripts in this repository may be a helpful reference.

@jhlegarreta
Copy link
Member Author

Would you rather me change the image to a smaller image in the Art folder or scale down the "itkLogo.png" image down 50%?

Ideally, the website should keep a stable size of the ITK logo, regardless of the original size of the image it uses, so I would discourage changing the file used by the website (as long as it has a reasonable size).

@viv511
Copy link
Contributor

viv511 commented Jul 24, 2024

Thanks everyone! Yes, I'm using a MB Pro (13-inch, M2, 2022) (ARM System).

To clarify, could I scale the current image via code rather than create a new file of the scaled logo?

The Doxygen repository provided has been helpful, but I'm still running into issues on step [4/7] @ [183/214]. I've provided the error code that I get. I have created a clone of the repository on my own machine to run the commands locally. I've also installed Docker and Doxygen so there should be no issues there.

.... (running code)
180.8 [182/214] Building CXX object src/CMakeFiles/doxymain.dir/language.cpp.o
180.8 ninja: build stopped: subcommand failed.
------
 - FromAsCasing: 'as' and 'FROM' keywords' casing do not match (line 1)
Dockerfile:18
--------------------
  17 |     
  18 | >>> RUN git clone https://github.com/doxygen/doxygen.git && \
  19 | >>>     ( cd doxygen && \
  20 | >>>       git checkout Release_1_9_3 && \
  21 | >>>       mkdir bld && cd bld && \
  22 | >>>       cmake -G Ninja -DCMAKE_BUILD_TYPE=Release .. && \
  23 | >>>       ninja install) && \
  24 | >>>     rm -rf ./doxygen
  25 |     
--------------------
ERROR: failed to solve: process "/bin/sh -c git clone https://github.com/doxygen/doxygen.git &&     ( cd doxygen &&       git checkout Release_1_9_3 &&       mkdir bld && cd bld &&       cmake -G Ninja -DCMAKE_BUILD_TYPE=Release .. &&       ninja install) &&     rm -rf ./doxygen" did not complete successfully: exit code: 1
Unable to find image 'itk-doxygen:latest' locally
docker: Error response from daemon: pull access denied for itk-doxygen, repository does not exist or may require 'docker login': denied: requested access to the resource is denied.

Let me know what next steps I should take. Thank you for your patience!

Best,
@viv511

@thewtex
Copy link
Member

thewtex commented Jul 24, 2024

To clarify, could I scale the current image via code rather than create a new file of the scaled logo?

Yes, as @jhlegarreta suggested, an HTML / CSS fix is probably the best, if possible.

Yes, I'm using a MB Pro (13-inch, M2, 2022) (ARM System).

Unable to find image 'itk-doxygen:latest' locally
docker: Error response from daemon:

It may be easier to get the doxygen executable from Homebrew or pixi.

@viv511 thank you for your persistence! What does seem at the surface to be a simple fix is turning out to be more of an adventure, but hopefully you find it to be a good learning experience.

@seanm
Copy link
Contributor

seanm commented Jul 24, 2024

It may be easier to get the doxygen executable from Homebrew or pixi.

You can just download a pre-built doxygen for Mac from https://www.doxygen.nl/files/Doxygen-1.11.0.dmg

@viv511
Copy link
Contributor

viv511 commented Jul 24, 2024

To clarify, could I scale the current image via code rather than create a new file of the scaled logo?

Yes, as @jhlegarreta suggested, an HTML / CSS fix is probably the best, if possible.

Yes, I'm using a MB Pro (13-inch, M2, 2022) (ARM System).

Unable to find image 'itk-doxygen:latest' locally
docker: Error response from daemon:

It may be easier to get the doxygen executable from Homebrew or pixi.

@viv511 thank you for your persistence! What does seem at the surface to be a simple fix is turning out to be more of an adventure, but hopefully you find it to be a good learning experience.

That makes sense, thanks!

Of course! I'm always ready to take on a new challenge and I'm having a fun time figuring stuff out. Thank you for your continued assistance! :)

I already downloaded Doxygen yesterday from their website. Running doxygen -v in my terminal prints out 1.11.0, which I believe is the same version you are referencing.

Are there any other places I should look into further? I can try reinstalling doxygen and seeing if that changes anything, although I don't think it will.

@N-Dekker
Copy link
Contributor

N-Dekker commented Jul 24, 2024

Running doxygen -v in my terminal prints out 1.11.0, which I believe is the same version you are referencing.

https://itk.org/Doxygen/html/ is still generated with Doxygen version 1.8.16.

Slightly related, I did use Doxygen version 1.11.0 to generate the elastix API documentation, at https://elastix.dev/doxygen/index.html Then we encountered a breaking change: "Modules" is renamed to "Topics", meaning that the old "Modules" menu item should be adjusted. No problem for now, just something to keep in mind when upgrading to Doxygen version 1.11.0!

@viv511
Copy link
Contributor

viv511 commented Jul 24, 2024

Oh I see, I'll try uninstalling/installing the older version.

@viv511
Copy link
Contributor

viv511 commented Jul 25, 2024

Hi all,

I have since done a deeper dive to understand how Docker/Doxygen/and all of the other technologies work together and I feel much better about how the commands and the scripts work together.

Here are the commands that I executed:

  1. docker build --build-arg TAG=v1.1.0 . -f Dockerfile -t itk-doxygen
    This command ran successfully and I was able to create a docker image & build successfully

  2. export TAG=v1.2.0
    Also successful; to use the TAG environment variable.

  3. The docker run command is where I ran into problems. I replaced the !ITK with the path to my own cloned directory as I presumed that it was a placeholder.

docker run \
  --env TAG \
  --mount type=bind,source=/Users/...myownpath.../ITK,destination=/work/ITK,readonly \
  --name itk-dox itk-doxygen

This command worked until it had to clone the ITKAdaptiveDenoising repository.

-- Performing Test have_sse2_extensions_var
-- Performing Test have_sse2_extensions_var - Failed
-- Performing Test CXX_HAS_DISABLE_OPTIMIZATION_FLAG
-- Performing Test CXX_HAS_DISABLE_OPTIMIZATION_FLAG - Failed
-- Found Python3: /usr/bin/python3.8 (found suitable version "3.8.10", minimum required is "3.8") found components: Interpreter
CMake Error at CMake/ITKModuleRemote.cmake:15 (message):
  Failed to clone repository:
  'https://github.com/ntustison/ITKAdaptiveDenoising.git'
Call Stack (most recent call first):
  CMake/ITKModuleRemote.cmake:138 (_git_clone)
  CMake/ITKModuleRemote.cmake:273 (_fetch_with_git)
  Modules/Remote/AdaptiveDenoising.remote.cmake:44 (itk_fetch_module)
  Modules/Remote/CMakeLists.txt:21 (include)


-- Configuring incomplete, errors occurred!
See also "/work/ITK-bld/CMakeFiles/CMakeOutput.log".
See also "/work/ITK-bld/CMakeFiles/CMakeError.log".

To troubleshoot, I tried to run these commands on the unaltered ITK cloned repository, I set the flags for that module to OFF (because I don't actually require that module for testing the main page of the documentation, so it should be okay to remove), and I thought it might be a problem with git parallel fetching, so I turned that off as well. However, none of these solved the issue.

Here are the Docker Logs for the docker run issue:

2024-07-25 11:15:06 Call Stack (most recent call first):
2024-07-25 11:15:06   CMake/ITKModuleRemote.cmake:138 (_git_clone)
2024-07-25 11:15:06   CMake/ITKModuleRemote.cmake:273 (_fetch_with_git)
2024-07-25 11:15:06   Modules/Remote/AdaptiveDenoising.remote.cmake:44 (itk_fetch_module)
2024-07-25 11:15:06   Modules/Remote/CMakeLists.txt:21 (include)

I believe the problem may lie with some git issue, but I'm not entirely sure. Following the error messages in the Docker logs seems to confirm this.

Help would be greatly appreciated! Let me know if I should provide more information.

Thank you so much for your patience and understanding,
@viv511

@blowekamp
Copy link
Member

I suspect the issue you are encountering is now due to the ITK repo directory is now readonly. So when git tries to clone into the readonly ITK source directory an error occours.

You may want to work in an interactive docker session to experiment. You may be able to run something like

docker run --rm -i -u $(id -u):$(id -g)
--mount type=bind,source=/Users/...myownpath.../ITK,destination=/work/ITK,readonly
--entrypoint /usr/bin/bash
--name itk-dox itk-doxygen

This will mount the source code into the docker as read/write and run the docker processes as you current user id.

@viv511
Copy link
Contributor

viv511 commented Aug 3, 2024

Hi! I removed the readonly part of the command and was able to build successfully. I then unzipped the .tar.gz file and was able to access the index.html file in order to build the documentation locally.

The change that I had originally proposed turned out to work quite well! Here is a before/after of the homepage of the documentation (Using chrome browser).

Before

Screenshot 2024-08-02 at 8 26 14 PM

After

Screenshot 2024-08-02 at 8 25 27 PM
Thank you to everyone who has helped me on this thread, I appreciate it so much! :D I've truly learned a lot through this process and will look through some other issues and try to help out where I can. I'll submit my change shortly.
Best, @viv511

@viv511
Copy link
Contributor

viv511 commented Aug 3, 2024

Hi! I made a PR in the ITKDoxygen repository as I saw some issues with the Markdown and instructions that I could fix based on my experience using it to test this change.

Best,
@viv511

@thewtex
Copy link
Member

thewtex commented Aug 7, 2024

@viv511 thank you for contributing! 🌮 🎉 🎇

@viv511
Copy link
Contributor

viv511 commented Aug 7, 2024

@viv511 thank you for contributing! 🌮 🎉 🎇

Thank you so much for the opportunity :) I learned a lot through this process! I'd love to continue to keep helping out.

@thewtex thewtex closed this as completed Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good first issue A good issue for community members new to contributing type:Documentation Documentation improvement or change
Projects
None yet
Development

No branches or pull requests

6 participants