Skip to content
This repository was archived by the owner on Apr 19, 2023. It is now read-only.

Conversation

@oskidan
Copy link

@oskidan oskidan commented Mar 28, 2022

Hi everyone,

This PR has been migrated from cpp-best-practices/cmake_template#7

It tries to solve the following issue: Linux and Windows configure presets cannot be used with Darwin-based platforms (such as macOS X). The discussion started in the ftxui_template project may be still relevant here.

@ddalcino
Copy link
Contributor

Thank you. I need to independently confirm that these work on my Mac, but I think this will be a quick approval from me.

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2022

Codecov Report

Merging #16 (0f36bfe) into main (ae64564) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main      #16   +/-   ##
=======================================
  Coverage   77.77%   77.77%           
=======================================
  Files           3        3           
  Lines          36       36           
  Branches       19       19           
=======================================
  Hits           28       28           
  Misses          7        7           
  Partials        1        1           
Flag Coverage Δ
Linux 37.03% <ø> (ø)
Windows 80.00% <ø> (ø)
macOS 38.46% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae64564...0f36bfe. Read the comment docs.

@ddalcino
Copy link
Contributor

Comment from the previous PR:

Can you please move this PR upstream to https://github.com/cpp-best-practices/cpp_boilerplate_project? That’s where this kind of thing should be discussed. Thanks.

OK, I will.

Are you able to use any of the Linux presets on Mac? You are right that we should support Mac better, but it surprises me that the Linux presets aren’t generic enough to work for you.

The presets themselves are generic enough. They can't be used on macOS because of this check: https://github.com/cpp-best-practices/ftxui_template/blob/69ccbc830fe8c81f81992e340aca081d90d98ba8/CMakePresets.json#L45-L49

If this check was modified to be "Linux" or "Darwin", everything would probably work fine. I don't know if VSCode CMake Tools allows that.

CMake presets supports an 'inList' operator that I think you could use. You can check if 'host os' is 'inList' that includes Linux and Mac. Docs here https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html#condition

Currently the only supported Unix-like OSes are:
- Linux;
- Darwin-based OSes, such as macOS X.
@oskidan
Copy link
Author

oskidan commented Mar 29, 2022

@ddalcino Great thanks, I did not realize that CMakePresets.json was actually a CMake thing, not a VSCode thing :-)

The PR was updated to have shared presets for Unix-like OSes. Should be possible to add BSD, MINIX, QNX, etc. into the list. Unfortunately I don't have any of them at hand to try.

@Jason5480
Copy link
Contributor

With this PR and the new condition, we support all three big OS! I do not have a macOS to check it out, but the changes are straight forward. Thank you @oskidan 👍
@ddalcino @lefticus I will rebase my PR #15 based on those changes after they get merged

@ddalcino
Copy link
Contributor

Should be possible to add BSD, MINIX, QNX, etc. into the list. Unfortunately I don't have any of them at hand to try.

I'm not certain, but I don't think we need to whitelist the operating systems for this preset. Maybe it would be sufficient to check that the host OS notEquals Windows. If we did that, we would allow support for any OS that has clang/g++/CMake/ninja. It wouldn't actively prevent users from using the presets, at least.

I don't want to go too far into testing every operating system out there; that's going to be super hard to support long-term.

What does everyone else think?

@oskidan
Copy link
Author

oskidan commented Mar 30, 2022

I think that there are a lot of operating systems out there. Some of them are not Windows-like and at the same time they are not Unix-like. For example FreeDOS, BeOS/Haiku. From my experience, the fact that an OS has the required toolchain, does not mean that the project can be built for it.

I'd prefer to have a list of officially supported OSes, so that I can at least reason about the amount of effort that will go into me using the library/software.

@ddalcino
Copy link
Contributor

I'd prefer to have a list of officially supported OSes, so that I can at least reason about the amount of effort that will go into me using the library/software.

Good point, you're probably right. Our Conan dependencies definitely do not support other OSes.

@Jason5480
Copy link
Contributor

I am also in favor of restricting the supported OSes in "big three". If you run an unsupported OS you still have the option to use dev container with this template. Thus, if you can install docker and VSCode (with Remote-Containers plugin) to the unsupported OS you can set up a working environment with one click! Another option someone has is to add CMakeUserPresets.json file for some unique use cases.

@lefticus lefticus requested a review from ddalcino March 31, 2022 15:10
@lefticus
Copy link
Member

@Jason5480 I've attempted to add you as a reviewer, but github is not allowing me to, for some reason I cannot figure out at the moment. But once you and @ddalcino agree this is ready, I'm happy to merge it, or @ddalcino can merge it.

Copy link
Contributor

@ddalcino ddalcino left a comment

Choose a reason for hiding this comment

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

Looks good to me. I have tested this manually on a Mac with VS Code, and everything is working fine.

@ddalcino
Copy link
Contributor

ddalcino commented Mar 31, 2022

@Jason5480 I've attempted to add you as a reviewer, but github is not allowing me to, for some reason I cannot figure out at the moment. But once you and @ddalcino agree this is ready, I'm happy to merge it, or @ddalcino can merge it.

@lefticus, I would love to merge this, but apparently, I do not have write access to the cpp_boilerplate_project. I can't even give an approving review that satisfies the review requirement.

@lefticus lefticus merged commit f37c1ab into cpp-best-practices:main Apr 1, 2022
@Jason5480 Jason5480 mentioned this pull request Apr 1, 2022
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants