Skip to content

Update bazel doxygen script [AP-3535] #154

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
May 23, 2025
Merged

Update bazel doxygen script [AP-3535] #154

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 12 additions & 13 deletions doxygen/doxygen.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
# EITHER EXPRESSED OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE IMPLIED
# WARRANTIES OF MERCHANTABILITY AND/OR FITNESS FOR A PARTICULAR PURPOSE.

load("//tools:configure_file.bzl", "configure_file_impl")
load("@rules_pkg//pkg:pkg.bzl", "pkg_zip")
load("//tools:configure_file.bzl", "configure_file_impl")

def _swift_doxygen_impl(ctx):
vars = ctx.attr.vars | {} # copy dict instead of referencing it
Expand All @@ -18,37 +18,36 @@ def _swift_doxygen_impl(ctx):
doxygen_out = ctx.actions.declare_directory(ctx.attr.name + "_doxygen")
vars["DOXYGEN_OUTPUT_DIRECTORY"] = doxygen_out.path

# this performs a CMake-like replacement of @VAR@ based on the vars dict
config = configure_file_impl(ctx, vars, ctx.attr.name + "_Doxyfile")[0].files.to_list()[0]

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"]

Comment on lines -23 to -30
Copy link
Contributor Author

@richarddeurloo richarddeurloo May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: these are now directly exposed as env variables in bash by setting env = vars below.

ctx.actions.run_shell(
inputs = [config] + ctx.files.deps,
outputs = [doxygen_out],
env = vars,
command = """
DOXYGEN_DOT_FOUND=NO
DOXYGEN_DOT_PATH=

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still use old doxygen config files?

Copy link
Contributor Author

@richarddeurloo richarddeurloo May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

sed -i "s|@DOXYGEN_DOT_FOUND@|$DOXYGEN_DOT_FOUND|g" {config}
sed -i "s|@DOXYGEN_DOT_PATH@|$DOXYGEN_DOT_PATH|g" {config}
sed -i "s|@PLANTUML_JAR_PATH@|/usr/local/bin/plantuml.jar|g" {config}
sed -i "s|@INPUT_DIR@|{input_dir}|g" {config}
sed -i "s|@PROJECT_NAME@|{project_name}|g" {config}
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}
Comment on lines +43 to +47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

@richarddeurloo richarddeurloo May 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


PATH=$PATH doxygen {config}
""".format(config = config.path, input_dir = input_dir, project_name = project_name),
""".format(config = config.path),
)

return [DefaultInfo(files = depset([doxygen_out, config]))]
Expand Down
Loading