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

actions: add step for checking miral and miroil symbols map #3336

Merged
merged 12 commits into from
Apr 19, 2024
48 changes: 48 additions & 0 deletions .github/workflows/symbols-check.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
name: Symbols Check

on:
push:
branches:
- main
pull_request:
types: [opened, synchronize, reopened, ready_for_review]

concurrency:
group: ${{ github.workflow }}-${{ github.event.number && format('pr{0}', github.event.number) || github.ref_name }}
cancel-in-progress: true

jobs:
Run:
runs-on: ubuntu-latest

timeout-minutes: 10
steps:
- name: Check out code
uses: actions/checkout@v4
with:
# So we can determine the merge base
fetch-depth: 0
Saviq marked this conversation as resolved.
Show resolved Hide resolved

- name: Install dependencies
run: |
sudo apt-get build-dep ./
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is the expensive step :)

Suggested change
sudo apt-get build-dep ./

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want the build deps so that 3rd party libraries resolve in clang, right?


- name: Configure
run: >
cmake -B build ${{ github.workspace }}

- name: Install clang
run: |
wget https://apt.llvm.org/llvm.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. What are the security concerns with this curl | sudo sh -?

It's in the GitHub runners infrastructure, so infrastructure escape shouldn't be a problem, but what does it have access to here?

I think the answer is that it's got access to the GitHub secrets? Which means it has access to the release signing key, and the signing key to upload to the PPA?

I don't think this should be a blocker here, but maybe we should record this somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We changed this to at least add it from the repository

chmod +x llvm.sh
sudo ./llvm.sh 19
Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, this appears to work correctly with clang-19 from 24.04 (once that becomes available in GH, I guess)

echo "MIR_SYMBOLS_MAP_GENERATOR_CLANG_SO_PATH=/usr/lib/llvm-19/lib/libclang.so.1" >> $GITHUB_ENV
echo "MIR_SYMBOLS_MAP_GENERATOR_CLANG_LIBRARY_PATH=/usr/lib/llvm-19/lib" >> $GITHUB_ENV

- name: Check miral symbols
run: |
cmake --build build --target check-miral-symbols-map

- name: Check miroil symbols
run: |
cmake --build build --target check-miroil-symbols-map
Saviq marked this conversation as resolved.
Show resolved Hide resolved
11 changes: 11 additions & 0 deletions src/miral/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -153,6 +153,17 @@ add_custom_target(
--output-symbols
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}")

add_custom_target(
check-miral-symbols-map
COMMAND ${PROJECT_SOURCE_DIR}/tools/symbols_map_generator/run.sh
--library-name miral
--version ${MIRAL_VERSION}
--symbols-map-path="${CMAKE_CURRENT_SOURCE_DIR}/symbols.map"
--external-headers-directory="${PROJECT_SOURCE_DIR}/include/miral"
--include-dirs "$<JOIN:$<TARGET_PROPERTY:miral,INCLUDE_DIRECTORIES>,:>"
--diff
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}")

configure_file(
${CMAKE_CURRENT_SOURCE_DIR}/miral.pc.in
${CMAKE_CURRENT_BINARY_DIR}/miral.pc
Expand Down
11 changes: 11 additions & 0 deletions src/miroil/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,17 @@ add_custom_target(
--output-symbols
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}")

add_custom_target(
check-miroil-symbols-map
COMMAND ${PROJECT_SOURCE_DIR}/tools/symbols_map_generator/run.sh
--library-name miroil
--version ${MIROIL_VERSION}
--symbols-map-path="${CMAKE_CURRENT_SOURCE_DIR}/symbols.map"
--external-headers-directory="${PROJECT_SOURCE_DIR}/include/miroil"
--include-dirs "$<JOIN:$<TARGET_PROPERTY:miroil,INCLUDE_DIRECTORIES>,:>"
--diff
WORKING_DIRECTORY "${PROJECT_SOURCE_DIR}")

install(TARGETS miroil LIBRARY DESTINATION "${CMAKE_INSTALL_LIBDIR}")
install(DIRECTORY ${CMAKE_SOURCE_DIR}/include/miroil DESTINATION "${CMAKE_INSTALL_INCLUDEDIR}")
install(FILES ${CMAKE_CURRENT_BINARY_DIR}/miroil.pc DESTINATION "${CMAKE_INSTALL_LIBDIR}/pkgconfig")
29 changes: 22 additions & 7 deletions tools/symbols_map_generator/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,14 +259,14 @@ def get_added_symbols(previous_symbols: list[Symbol], new_symbols: set[str], is_
return added_symbols


def print_symbols_diff(previous_symbols: list[Symbol], new_symbols: set[str], is_internal: bool):
def report_symbols_diff(previous_symbols: list[Symbol], new_symbols: set[str], is_internal: bool):
"""
Prints the diff between the previous symbols and the new symbols.
"""
added_symbols = get_added_symbols(previous_symbols, new_symbols, is_internal)

print("")
print("\033[1mNew Symbols 🟢🟢🟢\033[0m")
print(" \033[1mNew Symbols 🟢🟢🟢\033[0m")
for s in added_symbols:
print(f"\033[92m {s}\033[0m")

Expand All @@ -280,10 +280,12 @@ def print_symbols_diff(previous_symbols: list[Symbol], new_symbols: set[str], is
deleted_symbols.sort()

print("")
print("\033[1mDeleted Symbols 🔻🔻🔻\033[0m")
print(" \033[1mDeleted Symbols 🔻🔻🔻\033[0m")
for s in deleted_symbols:
print(f"\033[91m {s}\033[0m")

return len(deleted_symbols) > 0 or len(added_symbols) > 0


def main():
parser = argparse.ArgumentParser(description="This tool parses the header files of a provided in the Mir project "
Expand Down Expand Up @@ -327,6 +329,12 @@ def main():

args = parser.parse_args()
logging.basicConfig(level=logging.INFO)

if args.output_symbols:
_logger.info("symbols_map_generator is running in 'output symbols' mode")

if args.diff:
_logger.info("symbols_map_generator is running in 'diff' mode")

# Point libclang to a file on the system
if 'MIR_SYMBOLS_MAP_GENERATOR_CLANG_SO_PATH' in os.environ:
Expand Down Expand Up @@ -377,9 +385,14 @@ def main():

previous_symbols = read_symbols_from_file(args.symbols_map_path, library)

has_changed_symbols = False
if args.diff:
print_symbols_diff(previous_symbols, external_symbols, False)
print_symbols_diff(previous_symbols, internal_symbols, True)
print("External Symbols Diff:")
has_changed_symbols = report_symbols_diff(previous_symbols, external_symbols, False)

print("")
print("Internal Symbols Diff:")
has_changed_symbols = has_changed_symbols or report_symbols_diff(previous_symbols, internal_symbols, True)
print("")

if args.output_symbols:
Expand Down Expand Up @@ -466,8 +479,10 @@ def main():

f.write(output_str)


exit(0)
if has_changed_symbols:
exit(1)
else:
exit(0)
mattkae marked this conversation as resolved.
Show resolved Hide resolved



Expand Down
Loading