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

Applied clang-tidy suggestions for hyprland workspaces #2720

Merged
merged 2 commits into from
Dec 7, 2023

Conversation

zjeffer
Copy link
Contributor

@zjeffer zjeffer commented Dec 4, 2023

First test after merging #2595

Let me know if anything needs to change.

@zjeffer zjeffer force-pushed the master branch 2 times, most recently from a984ce5 to de46394 Compare December 4, 2023 14:06
@Alexays
Copy link
Owner

Alexays commented Dec 7, 2023

LGTM

@Alexays Alexays merged commit dc208b6 into Alexays:master Dec 7, 2023
7 of 8 checks passed
@zjeffer
Copy link
Contributor Author

zjeffer commented Dec 7, 2023

@Alexays FYI, I'm implementing the GitHub action for clang-tidy checks in this PR: zjeffer#1

I'm having some trouble here during the clang-tidy step (https://github.com/zjeffer/Waybar/actions/runs/7126784812/job/19405372955):

Performing checkup on src/modules/hyprland/workspaces.cpp
  INFO:CPP Linter:Running "clang-tidy-17 --export-fixes=.cpp-linter_cache/clang_tidy_output.yml -p /home/runner/work/Waybar/Waybar/build --extra-arg= src/modules/hyprland/workspaces.cpp"
  DEBUG:CPP Linter:Output from clang-tidy:
  /home/runner/work/Waybar/Waybar/src/modules/hyprland/workspaces.cpp:1:10: error: 'modules/hyprland/workspaces.hpp' file not found [clang-diagnostic-error]
      1 | #include "modules/hyprland/workspaces.hpp"
        |          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The problem is the project is not configured during this step, so we don't generate a compile_commands.json file. That's why it can't find the hpp file.

I tried using your waybar:debian image and configuring the project like this: https://github.com/zjeffer/Waybar/actions/runs/7120457195/workflow, but that caused issues with installing clang-tidy (https://github.com/zjeffer/Waybar/actions/runs/7120457195/job/19387733236):

Run if [[ "Linux"  == "Linux" ]]; then
  if [[ "Linux"  == "Linux" ]]; then
    sudo apt-get update
    # First try installing from default Ubuntu repositories before trying LLVM script
    if ! sudo apt-get install -y clang-format-17 clang-tidy-17; then
      # This LLVM script will add the relevant LLVM PPA: https://apt.llvm.org/
      wget https://apt.llvm.org/llvm.sh -O $GITHUB_ACTION_PATH/llvm_install.sh
      chmod +x $GITHUB_ACTION_PATH/llvm_install.sh
      if sudo $GITHUB_ACTION_PATH/llvm_install.sh 17; then
        sudo apt-get install -y clang-format-17 clang-tidy-17
      fi
    fi
  fi
  if [[ "Linux"  == "macOS" ]]; then
    python3 -m venv "$GITHUB_ACTION_PATH/venv"
    source "$GITHUB_ACTION_PATH/venv/bin/activate"
  fi
  python3 -m pip install -r "$GITHUB_ACTION_PATH/requirements.txt"
  clang-tools -i 17 -b
  shell: bash --noprofile --norc -e -o pipefail {0}
  env:
    GITHUB_TOKEN: ***
/__w/_temp/0cd2f190-16d6-4c82-aad3-0725a22ea861.sh: line 2: sudo: command not found
Error: Process completed with exit code 127.

To fix this, I would propose:

  • We add sudo to the waybar:debian image
  • We build a new container image specifically for configuring the project & running clang-tidy
  • We manually install sudo in the clang-tidy.yml file, before running the clang-tidy step

What do you suggest?

@Alexays
Copy link
Owner

Alexays commented Dec 7, 2023

We can add sudo to images, Dockerfile are also stored in the repo, once changed i'll trigger a rebuild :)

@zjeffer
Copy link
Contributor Author

zjeffer commented Dec 7, 2023

I'm doing some manual tests by adding sudo and python3-pip (which also seems needed) in the yml file. I'll make a PR to change the Dockerfile once I've figured out how to fix the next errors I'm getting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants