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.

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

Jump to bottom

Feature/support macos builds #66

Closed
wants to merge 5 commits into from
Closed

Feature/support macos builds #66

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cb30b2b
Add support to build on OSX with clang-18 or newer
ClausKlein Nov 10, 2024
3cb30e8
Quickfix
ClausKlein Nov 10, 2024
4c36c59
Add more FIXME notes
ClausKlein Nov 10, 2024
904c7ed
Merge branch 'main' into feature/support-macos-builds
ClausKlein Nov 12, 2024
4cc47f3
Revert my workaround and notes
ClausKlein Nov 12, 2024
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
57 changes: 30 additions & 27 deletions .github/workflows/ci_tests.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,39 +31,32 @@ jobs:
strategy:
fail-fast: false
matrix:
platform: [ubuntu-latest]
platform: [ubuntu-24.04, macos-15]
compiler:
- cpp: g++
c: gcc
- cpp: clang++
c: clang
- cpp: g++-14
c: gcc-14
- cpp: clang++-18
c: clang-18
cpp_version: [17, 20, 23, 26]
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
cmake_args:
- description: "Default"
args: ""
- description: "TSan"
args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread"
- description: "ASan"
args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'"
# FIXME: find a better solution to be usable for multible OS builds on CI! CK
# - description: "TSan"
# args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread"
# - description: "ASan"
# args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'"
Copy link
Author

Choose a reason for hiding this comment

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

better solution may be found here: https://github.com/beman-project/execution26/tree/main/.github/workflows

Copy link
Member

Choose a reason for hiding this comment

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

Can you provide more description here? What are you trying to do?

Copy link
Member

Choose a reason for hiding this comment

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

Hey CK, I would suggest you defer further improvement to another PR. Leaving a note here with some explanation on what improvements would be needed should be sufficient.

This PR is currently already complex and changes suggested to the build system are affecting fundamentals to our setup, it may be best to leave this PR in a stable state so other beman project members can perform reviews. As it may take some time for members to discuss the implications of the build updates.

Having small, atomic PRs also help all of us manage changes easily, as the longer this PR is hanging in the review queue, the more out of date this PR is in respect to other pending improvements, this applies to both sides.

Again thank you for your contribution.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, at the moment, I have no plan to change more on this PR.
But there are to FIXME in my changes and I have no advice how to continue?

For me, your CI workflow tries to match.

I would split the CI in OS specific parts, reduce the compiler versions, and the C++ standards you want to check.
Also I would also create CI specific cmake workflow presets and use them.

see too CMakeWorkflowPresets: How to keep it as simple as possible?

include:
- platform: ubuntu-latest
- platform: ubuntu-22.04
compiler:
cpp: g++
c: gcc
cpp_version: 17
cmake_args:
description: "Werror"
args: "-DCMAKE_CXX_FLAGS='-Werror=all -Werror=extra'"
- platform: ubuntu-latest
compiler:
cpp: g++
c: gcc
cpp_version: 17
cmake_args:
description: "Dynamic"
args: "-DBUILD_SHARED_LIBS=on"
- description: "Werror"
args: "-DCMAKE_CXX_FLAGS='-Werror=all -Werror=extra'"

name: "Bulid & Test: ${{ matrix.compiler.c }} ${{ matrix.cpp_version }} ${{ matrix.cmake_args.description }}"
name: "${{ matrix.platform }}: ${{ matrix.compiler.c }} ${{ matrix.cpp_version }} ${{ matrix.cmake_args.description }}"
runs-on: ${{ matrix.platform }}
steps:
- uses: actions/checkout@v4
Expand All @@ -74,31 +67,41 @@ jobs:
ninjaVersion: "^1.11.1"
- name: Print installed softwares
run: |
wusatosi marked this conversation as resolved.
Show resolved Hide resolved
clang++ --version
g++ --version
${{ matrix.compiler.cpp }} --version || echo ignored
${{ matrix.compiler.c }} --version || echo ignored
cmake --version
ninja --version

- name: Configure CMake on macos with clang
if: startsWith(matrix.compiler.c, 'clang') && startsWith(matrix.platform, 'macos')
run: |
CC=$(brew --prefix llvm@18)/bin/clang CXX=$(brew --prefix llvm@18)/bin/clang++ cmake -B build -S . -DCMAKE_CXX_STANDARD=${{ matrix.cpp_version }} ${{ matrix.cmake_args.args }}
env:
CMAKE_GENERATOR: "Ninja Multi-Config"

- name: Configure CMake
if: ( startsWith(matrix.compiler.c, 'gcc') || startsWith(matrix.platform, 'ubuntu') ) || ( startsWith(matrix.compiler.c, 'gcc') && startsWith(matrix.platform, 'macos') )
run: |
cmake -B build -S . -DCMAKE_CXX_STANDARD=${{ matrix.cpp_version }} ${{ matrix.cmake_args.args }}
env:
CC: ${{ matrix.compiler.c }}
CXX: ${{ matrix.compiler.cpp }}
CMAKE_GENERATOR: "Ninja Multi-Config"

- name: Build Release
run: |
cmake --build build --config Release --verbose
cmake --build build --config Release --target all_verify_interface_header_sets
cmake --install build --config Release --prefix /opt/beman.exemplar
find /opt/beman.exemplar -type f
cmake --install build --config Release --prefix /tmp/beman.exemplar
find /tmp/beman.exemplar -type f
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to keep /opt? It's usually a path used for Linux libraries and I think that path should also exist for MAC OS.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but /opt is owned by root.
I would use ${PWD}/stagedir like boost does.

Copy link
Member

Choose a reason for hiding this comment

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

I think we supposed regardless of who is owner of /opt, all users can create a subdirectory or whatever.

My point is that /opt is used in all repos in Beman right now in documentation. If this recommendation is not OK, I'm OK to change it everywhere (other PRs for docs), but if it's works, I won't do the change.

I will also test and come back.

Copy link
Author

Choose a reason for hiding this comment

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

from CI build on MAC OS:

ld: warning: ignoring duplicate libraries: 'lib/Release/libgtest.a'
[1/1] Building CXX object src/beman/exemplar/CMakeFiles/beman.exemplar_verify_interface_header_sets.dir/Release/beman.exemplar_verify_interface_header_sets/beman/exemplar/identity.hpp.cxx.o
CMake Error at build/src/beman/exemplar/cmake_install.cmake:44 (file):
  file cannot create directory: /opt/beman.exemplar/include/beman/exemplar.
  Maybe need administrative privileges.
Call Stack (most recent call first):
  build/cmake_install.cmake:42 (include)


Error: Process completed with exit code 1.

Copy link
Member

@neatudarius neatudarius Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

$ ls -l / | grep /opt
drwxr-xr-x   7 dariusn root       4096 Nov 12 07:38 opt

I guess it depends if and how you have configured /opt. So we may add:

# Create the /opt directory if it doesn’t already exist
sudo mkdir -p /opt

# Change ownership of /opt to the current user
sudo chown -R $USER /opt

(check other existing commands, sudo may not be required!)

I would use ${PWD}/stagedir like boost does.
I would keep the current PR just for adding MAC OS support. We can discuss on discourse (https://discourse.bemanproject.org/latest) in a new post if /opt is or not the best path to recommend to install in docs, etc CI. So I'm OK with changing it, but the flow should be:

  • discourse thread and get a vote
  • add a recommendation into Beman docs (beman/ repo)
  • the propagate to ALL repos (their config and docs)
    We should aim to have the same default path in all places/repos! (docs, CI files, etc). It just makes the usability simpler for new users! And it still allows folks to use other custom install path on own system!

Copy link
Author

@ClausKlein ClausKlein Nov 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It does not successfully run on Darwin if we install to /opt?

As a user of project, I want to have control if and where the binaries are installed!

I am not willing to change more.

Copy link
Member

Choose a reason for hiding this comment

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

/opt is the correct location in a unix FHS style system because every other prefix is owned by someone else. / and /usr (now fused anyway) are owned by the OS distro, /usr/local is the machine admin, ~ and ~/.local are user controlled.
https://www.pathname.com/fhs/pub/fhs-2.3.html#OPTADDONAPPLICATIONSOFTWAREPACKAGES

"A package to be installed in /opt must locate its static files in a separate /opt/ or /opt/ directory tree, where is a name that describes the software package and is the provider's LANANA registered name."

So a todo to register beman with LANANA, although it's actually unlikely real world installs go there.

Testing install in CI is also not locking in an install location for any user. PREFIX is controlled by the installer.

That all said, outside of a scratch VM like a GitHub action runs in, using a directory inside out root, but outside the source tree, like we do for build, is the friendly way to do it.

Staging isn't the right name, because it's not going anywhere after that. Trying to do a package manager's job is a mistake

Copy link
Member

Choose a reason for hiding this comment

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

As a user of project, I want to have control if and where the binaries are installed!

I am not willing to change more.
@ClausKlein , I agree with you, as a user you can install them anywhere you want. As a developer inside the Beman project, you must use some requirements/recommendations/best practices used inside the Beman org. That's not questionable, but you are more than welcome to propose to change / upgrade/ improves these sets of standards or practices.

TLDR:

  • This CI config is exclusively for internal development. It must follow our internal practices or rules.
  • Similarly, examples for root README.md, are just examples and our recommendations. Our choice, but not an obligation for users.
  • Any user can install our libraries in a custom path, that's for sure an available option!

Steve also explained this in previous comment - "Testing install in CI is also not locking in an install location for any user. PREFIX is controlled by the installer."

I hope that is clear now. Please revert the changes related to the install path. Thanks!

- name: Test Release
run: ctest --test-dir build --build-config Release
- name: Build Debug
run: |
cmake --build build --config Debug --verbose
cmake --build build --config Debug --target all_verify_interface_header_sets
cmake --install build --config Debug --prefix /opt/beman.exemplar
find /opt/beman.exemplar -type f
cmake --install build --config Debug --prefix /tmp/beman.exemplar
find /tmp/beman.exemplar -type f
- name: Test Debug
run: ctest --test-dir build --build-config Debug

Expand Down
8 changes: 7 additions & 1 deletion .gitignore
View file
Open in desktop
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -1,2 +1,8 @@
/compile_commands.json
#
# prevent in source build!
#
/CMakeCache.txt
/CMakeFiles/
/CMakeUserPresets.json
/build
/stagedir
wusatosi marked this conversation as resolved.
Show resolved Hide resolved
22 changes: 15 additions & 7 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

cmake_minimum_required(VERSION 3.25)
cmake_minimum_required(VERSION 3.25...3.31)

project(
beman.exemplar # CMake Project Name, which is also the name of the top-level
Expand All @@ -9,9 +9,17 @@ project(
LANGUAGES CXX
)

include(CTest)
if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
message(FATAL_ERROR "In-source builds are not allowed!")
endif()

include(CPack)
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
include(FetchContent)
include(GNUInstallDirs)

add_subdirectory(src/beman/exemplar)

option(BUILD_TESTING "build tests too" ${PROJECT_IS_TOP_LEVEL})
Copy link
Member

Choose a reason for hiding this comment

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

The recommended pattern for options is to include the name of the project, so that projects can compose. Defaulting to is top level is the right thing, but we wouldn't want to build the tests of beman_interface if we included that.
Was going to come back to fix, but if we're touching this, might as well.

This comment was marked as resolved.

Sign in to view

if(BUILD_TESTING)
enable_testing()

Expand All @@ -26,14 +34,14 @@ if(BUILD_TESTING)
block()
set(INSTALL_GTEST OFF) # Disable GoogleTest installation
set(BUILD_TESTING OFF) # Disable GoogleTest tests
set(BUILD_GMOCK OFF)
set(CMAKE_CXX_FLAGS) # FIXME: Do not propagate our project settings! CK
Copy link
Member

Choose a reason for hiding this comment

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

Propagating the project settings is exactly what we want to do. It's why gtest recommends vendoring rather than packaging.

Copy link
Author

Choose a reason for hiding this comment

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

What if your build dependencies doesn't compile with your compiler flags?

Copy link
Member

Choose a reason for hiding this comment

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

Then the code is likely fundamentally incompatible? Except for warning flags, pretty much all other flags are detectable changes that will compile differently, leading to ODR violations, sometimes just harmless. This is true even for the standard library to some extent, although standard library implementors go to great lengths to avoid such breaks.
Feature test macros and polyfills are common examples. Standard library implementation choice (libc++ vs libstdc++) is another.
It's possible that some flags are peculiar to a package, or there are exceptional workarounds necessary, but by default all C++ in a link context should be compiled with the same flags.

Copy link
Author

Choose a reason for hiding this comment

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

see #66 (comment)

FetchContent_MakeAvailable(googletest)
endblock()
endif()

add_subdirectory(src/beman/exemplar)

if(BUILD_TESTING)
add_subdirectory(tests/beman/exemplar)
endif()

add_subdirectory(examples)
if(PROJECT_IS_TOP_LEVEL)
add_subdirectory(examples)
endif()
14 changes: 11 additions & 3 deletions CMakePresets.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"hidden": true,
"cacheVariables": {
"CMAKE_BUILD_TYPE": "Debug",
"CMAKE_CXX_FLAGS": "-fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=leak -fsanitize=undefined"
"CMAKE_CXX_FLAGS": "-fsanitize=address -fsanitize=pointer-compare -fsanitize=pointer-subtract -fsanitize=undefined"
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
}
},
{
Expand Down Expand Up @@ -52,11 +52,19 @@
"buildPresets": [
{
"name": "gcc-debug",
"configurePreset": "gcc-debug"
"configurePreset": "gcc-debug",
"targets": [
"all_verify_interface_header_sets",
"all"
]
},
{
"name": "gcc-release",
"configurePreset": "gcc-release"
"configurePreset": "gcc-release",
"targets": [
"all_verify_interface_header_sets",
"all"
]
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
}
],
"testPresets": [
Expand Down
4 changes: 1 addition & 3 deletions src/beman/exemplar/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception

add_library(beman.exemplar STATIC)
add_library(beman.exemplar INTERFACE)
ClausKlein marked this conversation as resolved.
Show resolved Hide resolved
add_library(beman::exemplar ALIAS beman.exemplar)

target_sources(beman.exemplar PRIVATE identity.cpp)

target_sources(
beman.exemplar
PUBLIC
Expand Down