Feature/support macos builds #66
Conversation
ClausKlein
requested review from
bretbrownjr,
camio,
dietmarkuehl,
neatudarius and
steve-downey
as code owners
ClausKlein
force-pushed
the
feature/support-macos-builds
branch
from
45ddede to
09b7adb
Compare
|
note: Should close: #22 after merging. |
ClausKlein
force-pushed
the
feature/support-macos-builds
branch
from
66a286b to
beb59c0
Compare
ClausKlein
force-pushed
the
feature/support-macos-builds
branch
3 times, most recently
from
c8152df to
1116717
Compare
There was a problem hiding this comment.
Again thank you so much for the PR, CI testing is painful on GitHub so thank you for keeping the commit log clean, we can also squash merge it when it's done if you don't want to keep squashing your commits (I presume that's what all the force pushes are for).
Suggested changes:
- remove the ubuntu 20.04 comment from Ubuntu-latest
- revert the minimum version back to 17
- leave some documentation for the commented-out code, or remove them entirely.
The changes to the CI looks good to me besides the suggestions. The changes to the CMake files looks like great improvements, but I am not qualified to review them. Code owners mentioned in the PR would be better candidates to evaluate those changes.
|
|
||
| add_subdirectory(src/beman/exemplar) | ||
|
|
||
| option(BUILD_TESTING "build tests too" ${PROJECT_IS_TOP_LEVEL}) |
There was a problem hiding this comment.
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.
This comment was marked as resolved.
Sorry, something went wrong.
|
I presume by comment: #64 (comment) This PR packs a fix for #64 as well? |
ClausKlein
force-pushed
the
feature/support-macos-builds
branch
from
1116717 to
ebde17f
Compare
This is an interface library (header only) Prevent in source build Fix macos CI build Use all_verify_interface_header_sets on CI too. Co-authored-by: Darius Neațu <[email protected]> Co-authored-by: River <[email protected]>
ClausKlein
force-pushed
the
feature/support-macos-builds
branch
from
c821774 to
cb30b2b
Compare
.github/workflows/ci_tests.yml
Outdated
| # 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'" |
There was a problem hiding this comment.
better solution may be found here: https://github.com/beman-project/execution26/tree/main/.github/workflows
There was a problem hiding this comment.
Can you provide more description here? What are you trying to do?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
CMakeLists.txt
Outdated
| @@ -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 | |||
There was a problem hiding this comment.
Propagating the project settings is exactly what we want to do. It's why gtest recommends vendoring rather than packaging.
There was a problem hiding this comment.
What if your build dependencies doesn't compile with your compiler flags?
There was a problem hiding this comment.
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.
.github/workflows/ci_tests.yml
Outdated
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, but /opt is owned by root.
I would use ${PWD}/stagedir like boost does.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
$ ls -l / | grep /opt
drwxr-xr-x 7 dariusn root 4096 Nov 12 07:38 optI 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/optis 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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
/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
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Thanks for contribution @ClausKlein!
I'm temporary blocking this PR until we finish pending discussions, mostly related to Beman org practices. Thanks for understanding!
| - description: "ASan" | ||
| args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'" | ||
| args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'" |
There was a problem hiding this comment.
| args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'" | |
| args: "-DCMAKE_CXX_FLAGS='-fsanitize=address -fsanitize=undefined'" |
| cpp_version: [17, 20, 23, 26] | ||
| cmake_args: | ||
| - description: "Default" | ||
| args: "" | ||
| - description: "TSan" | ||
| args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread" | ||
| args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread" |
There was a problem hiding this comment.
| args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread" | |
| args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread" |
|
@wusatosi I closed this PR because with only one CI workflow file, it seems to me to complicated to support Windows, OSX, and First of all, the |
I don't get what are you trying to say here. You are free to create a separate CI file/ separate CI job under the same file. I don't have opinions with either approaches, I will go with whatever reviewers would prefer, #80 is still pending and I am fine to the change MSVC related change to another CI job/ another workflow. My personal take is. The pain in declaring matrixes is the same if its within one file or across multiple files, having it in the same file restricts the format of the matrix so its easier to tell what platform is running what set of build & test. There's less chance that one set of matrix is missing from anther platform due to not checking across files. It also allows downstream to copy-paste that one file to launch another set of matrix easily, with clear examples on how to setup specific build commands on each platform. Regarding first point: What does this have to do with workflows? What toolchain files are you refereeing to. |
|
Can I assume you are not going to take this PR forward? @ClausKlein |
Edit from river:
Closes #22 .
Closes #64 .
Closes #65 .
Closes #67 .