Update bazel doxygen script [AP-3535] #154
Conversation
| sed -i "s|@INPUT_DIR@|$PROJECT_SOURCE_DIR|g" {config} | ||
| sed -i "s|@PROJECT_NAME@|$PROJECT_NAME|g" {config} | ||
| sed -i "s|@STABLE_GIT_TAG@|$STABLE_GIT_TAG|g" {config} | ||
| sed -i "s|@DOXYGEN_EXCLUDE@|$DOXYGEN_EXCLUDE|g" {config} | ||
| sed -i "s|@PROJECT_SOURCE_DIR@|$PRW|g" {config} |
There was a problem hiding this comment.
The following issue is not related to your PR, but to the way this script is implemented in the first place. I believe the doxygen actions are not correctly setup to be reproducible on macos.
I wanted to test this new version on integrity doxygens, and it turns out this sed command does not seem to be compatible with macos sed, which requires an argument to the -i option.
Using the following patch:
platform=$(uname)
if [ "$platform" = "Darwin" ]; then
SED='sed -i ""'
else
SED="sed -i "
fi
// Updated sed calls
$SED "s|@DOXYGEN_DOT_FOUND@|$DOXYGEN_DOT_FOUND|g" $config
I then get in-place editing only works for regular files because the doxygen files are links, not regular files.
So I updated the calls again to:
config=$(readlink -f {config})
# backward compatibility with old CMake-style doxygen config files
$SED "s|@DOXYGEN_DOT_FOUND@|$DOXYGEN_DOT_FOUND|g" $config
Then doxygen is not found (I have it installed, but PATH seems to be cleared when running this script).
/bin/bash: line 28: doxygen: command not found
// $PATH value is the following, bazel probably isolates it from my user PATH
/usr/gnu/bin:/usr/local/bin:/bin:/usr/bin:.
There was a problem hiding this comment.
@richarddeurloo @sbmueller Then my question is, are you able to build doxygens on your mac? Should we find a way to make this script platform independent, and add doxygen binaries as explicit dependencies?
There was a problem hiding this comment.
@armallen, I agree and this script is a thorn in my eye as well. I don't have time to clean this up now. I just wanted to "make it work" for the moment. We have another ticket to deal with the refactor: https://swift-nav.atlassian.net/browse/AP-3482
And you are right about the sed and doxygen issues on Mac. This is an existing problem from the script. The only way around it for the moment is to run it in a docker container on Mac. I've tested it and it works. See my comment in https://github.com/swift-nav/starling-core/pull/898: For bazel: works-in-local-docker™️. 😬
docker run --rm --name doxygen -it --platform linux/amd64 --entrypoint /bin/bash -v $(pwd):/mnt/workspace/starling-core/ 571934480752.dkr.ecr.us-west-2.amazonaws.com/swift-build-modern:2025-02-19
There was a problem hiding this comment.
@armallen I've stumbled upon the exact same issues. I took a look a few weeks ago (see my comments in above ticket) and I can't help but realize the doxygen bazel implementation is a mere PoC for Linux which I think never was intended as a final and general solution. This definitely needs work.
There was a problem hiding this comment.
Ok it makes sense!
I'll test it in orion-engine's linux dev workspace then, there's no hurry to provide devs a way to generate local doxygen for now
| input_dir = "" | ||
| if "PROJECT_SOURCE_DIR" in vars: | ||
| input_dir = vars["PROJECT_SOURCE_DIR"] | ||
|
|
||
| project_name = "" | ||
| if "PROJECT_NAME" in vars: | ||
| project_name = vars["PROJECT_NAME"] | ||
|
|
There was a problem hiding this comment.
Note: these are now directly exposed as env variables in bash by setting env = vars below.
| if command -v dot &> /dev/null | ||
| then | ||
| DOXYGEN_DOT_FOUND=YES | ||
| DOXYGEN_DOT_PATH=$(which dot) | ||
| fi | ||
|
|
||
| PRW=`pwd` | ||
|
|
||
| # backward compatibility with old CMake-style doxygen config files |
There was a problem hiding this comment.
Do we still use old doxygen config files?
There was a problem hiding this comment.
What I meant here was the use of @VAR@ in the config files. Unfortunately, I had to maintain those in the new Doxygen config files. Otherwise the CMake builds would fail. So it was either maintain it here (for the moment) or have duplicate config files, for CMake and Bazel. That didn't seem like the best approach. I hope we can drop CMake at some point and use the default Doxygen approach $(VAR).
This PR support the templated Doxygen configuration files added to the hummingbird repo, which are used in starling-core. This PR adds support for a number of new templated variables.
Related PR: