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

Feature/support macos builds #66

Closed

Conversation

ClausKlein
Copy link

@ClausKlein ClausKlein commented Nov 1, 2024

Edit from river:
Closes #22 .
Closes #64 .
Closes #65 .
Closes #67 .

@wusatosi
Copy link
Member

wusatosi commented Nov 8, 2024

note: Should close: #22 after merging.

@ClausKlein ClausKlein marked this pull request as ready for review November 8, 2024 18:52
Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, it looks like this PR is still a work in progress.

Please make sure the lowest C++ version in CI is C++17.

Would also recommend you provide more context for this pr.

.github/workflows/ci_tests.yml Outdated Show resolved Hide resolved
.github/workflows/ci_tests.yml Show resolved Hide resolved
.gitignore Show resolved Hide resolved
@ClausKlein ClausKlein force-pushed the feature/support-macos-builds branch 3 times, most recently from c8152df to 1116717 Compare November 8, 2024 21:42
Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

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.

.github/workflows/ci_tests.yml Outdated Show resolved Hide resolved
.github/workflows/ci_tests.yml Outdated Show resolved Hide resolved
CMakeLists.txt Show resolved Hide resolved

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.

CMakePresets.json Show resolved Hide resolved
src/beman/exemplar/CMakeLists.txt Show resolved Hide resolved
@wusatosi
Copy link
Member

wusatosi commented Nov 9, 2024

I presume by comment: #64 (comment)

This PR packs a fix for #64 as well?
Is this why we are using interface build?

Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

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

LGTM for current status, but I have some small observations

.gitignore Show resolved Hide resolved
Copy link
Member

@wusatosi wusatosi left a comment

Choose a reason for hiding this comment

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

CI part looks good to me besides the suggestions (nitpicks) above.

Thanks for the contribution.

CMakeLists.txt Outdated Show resolved Hide resolved
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]>
Comment on lines 44 to 48
# 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.

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?

@ClausKlein ClausKlein marked this pull request as draft November 10, 2024 20:13
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
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.

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

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

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!

@ClausKlein ClausKlein marked this pull request as ready for review November 12, 2024 05:29
Copy link
Member

@neatudarius neatudarius left a comment

Choose a reason for hiding this comment

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

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'"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread"
args: "-DCMAKE_CXX_FLAGS=-fsanitize=thread"

@ClausKlein
Copy link
Author

@wusatosi I closed this PR because with only one CI workflow file, it seems to me to complicated to support Windows, OSX, and Linux.

First of all, the cmake workflow presets and maybe the toolchain files must be available.
Second, it must be clear how to create the cmake export config package.

@wusatosi
Copy link
Member

wusatosi commented Nov 25, 2024

@wusatosi I closed this PR because with only one CI workflow file, it seems to me to complicated to support Windows, OSX, and Linux.

First of all, the cmake workflow presets and maybe the toolchain files must be available. Second, it must be clear how to create the cmake export config package.

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.
Second point: What do you mean how to create the cmake export config package? are you referring to #31 ? I struggle to see relevance here, can you provide more description?

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