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

Adds AppleClang + msvc preset #82

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

wusatosi
Copy link
Member

@wusatosi wusatosi commented Nov 20, 2024

Added preset in accordance to discourse topics:

  • AppleClang-debug: Address sanitizer set besides leak sanitizer
  • AppleClang-release: O3 with debug info
  • msvc-debug: Address sanitizer only (other santizier not supported)
  • msvc-release: /O2 (highest performance optimization)

Appropriate CI testings are added.

closes #65

@wusatosi wusatosi changed the title Adds xcode + xcode preset Adds xcode + msvc preset Nov 20, 2024
CMakePresets.json Outdated Show resolved Hide resolved
Copy link
Contributor

@camio camio left a comment

Choose a reason for hiding this comment

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

Some minor tweaks suggested. I tested this on a MacOS and Windows system directly and it worked well.

CMakePresets.json Outdated Show resolved Hide resolved
],
"cacheVariables": {
"CMAKE_CXX_COMPILER": "g++"
}
},
{
"name": "gcc-release",
"displayName": "GCC Release Build",
"name": "xcode-debug",
Copy link
Contributor

Choose a reason for hiding this comment

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

XCode is an IDE, not a compiler. Unless your intent is to use the Xcode generator, this should be called something like apple_clang-debug since it targets Apple's clang fork

If there are no differences between supporting apple clang and stock clang then maybe "clang-debug" is a good name.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a reason to separate apple clang out in respect to clang-clang, it is to prevent scenaior like: #65 .

The main pain-point is sanitizers.

Copy link
Member Author

@wusatosi wusatosi Dec 5, 2024

Choose a reason for hiding this comment

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

I am simply implementing your initial proposal.

image

I take that your initial proposal is to support xcode based toolchain. Apple-clang is just part of it (we will likly add whatever the etc part is going to be in the future).

The intention here is not to soly support apple clang but extra configurations that might arise from xcode based toolchains.

Let me know if I am interpreting your initial proposal incorrectly.

Your proposal is linked above as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see the confusion now. My proposal was suggesting that the Generator be "XCode" for the MacOS platform and that the Generator be "Visual Studio 2022" for the Windows platform.

What you've done here is use "Ninja" as the generator for these platforms. I think that's fine for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

AppleClang-debug?

Yep, that sounds good.

For compiler names we could potentially follow the convention here. That implies we would change gcc-release to GNU-release which may be surprising for those not familiar with that list, but at least we'd have something to reference.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am not too too keen on gnu-debug... GCC is self descriptive enough and don't need to consult a chart...

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I'm with you there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member Author

Choose a reason for hiding this comment

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

hum, should I update msvc-debug to MSVC-debug, or should I use appleclang-debug?

CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
CMakePresets.json Outdated Show resolved Hide resolved
Copy link
Contributor

@camio camio left a comment

Choose a reason for hiding this comment

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

CMAKE_CXX_FLAGS should be removed from _release-base per prior discussion.

@wusatosi
Copy link
Member Author

wusatosi commented Dec 6, 2024

CMAKE_CXX_FLAGS should be removed from _release-base per prior discussion.

Wait presets are for end users. Any upstream that consumes our library as a dependency wouldn't get any CMAKE_CXX_FLAGS because we include this definition in presets. I don't think this is applicable to prior discussion, we are the site of linking, the beman project is the project the invoker is working on.

If the idea for preset is that it is a substitute for the generic setup script of:

cmake -S . -B build -DCMAKE_CXX_STANDARD=20 
cmake --build build
ctest --test-dir build

Leaving c++ version undefined will let to varying development environment. It would like saying the build command for the project is:

cmake -S . -B build
cmake --build build
ctest --test-dir build

This may lead to tests that are disabled for some while enabled for others.

@camio
Copy link
Contributor

camio commented Dec 11, 2024

CMAKE_CXX_FLAGS should be removed from _release-base per prior discussion.

Wait presets are for end users.

Oops, I intended to say they should be moved from _release-base (which has non-compiler-specific settings) to gcc-release and xcode-release (which has compiler-specific settings).

@wusatosi
Copy link
Member Author

CMAKE_CXX_FLAGS should be removed from _release-base per prior discussion.

Wait presets are for end users.

Oops, I intended to say they should be moved from _release-base (which has non-compiler-specific settings) to gcc-release and xcode-release (which has compiler-specific settings).

Okay this make sense 👌

@wusatosi
Copy link
Member Author

CMAKE_CXX_FLAGS should be removed from _release-base per prior discussion.

Wait presets are for end users.

Oops, I intended to say they should be moved from _release-base (which has non-compiler-specific settings) to gcc-release and xcode-release (which has compiler-specific settings).

Done

@wusatosi wusatosi requested a review from camio December 11, 2024 17:26
@wusatosi wusatosi changed the title Adds xcode + msvc preset Adds AppleClang + msvc preset Dec 11, 2024
Copy link
Contributor

@camio camio left a comment

Choose a reason for hiding this comment

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

One minor suggestion. Otherwise looks good to me.

CMakePresets.json Outdated Show resolved Hide resolved
Co-authored-by: David Sankel <[email protected]>
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.

clang++: error: unsupported option '-fsanitize=leak' for target 'x86_64-apple-darwin23.6.0'
4 participants