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

CMake build with find_package support and automatic translation. #15

Closed
wants to merge 7 commits into from
Closed

Conversation

alexreinking
Copy link

@alexreinking alexreinking commented Sep 24, 2022

This PR adds CMake build and packaging support... correctly. Users can simply build and install cppfront via:

$ cmake -S . -B build -DCMAKE_BUILD_TYPE=Release
$ cmake --build build --target install

and then write a simple CMakeLists.txt for their project:

cmake_minimum_required(VERSION 3.23)
project(cpp2-example)

find_package(cppfront REQUIRED)
# add_subdirectory(cppfront)
# FetchContent, etc.

add_executable(main main.cpp2)

There's a full example in example/, too. Users who don't want the cpp2 autodetection can set CPPFRONT_NO_MAGIC to 1 before find_package and can use cppfront_enable(TARGETS main) instead. Alternatively, they can get translated source lists manually via cppfront_generate_cpp(srcs_var <file.cpp2...>).


It also augments the test suite to actually build the generated CPP1 files and corrects the tests in response to this. Working on Windows with MSVC and Linux with GCC11. macOS has issues (there's another PR with this). All of this is tested in GitHub Actions workflows.

Copy link
Contributor

@JohelEGP JohelEGP left a comment

Choose a reason for hiding this comment

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

This is excellent.

It also augments the test suite to actually build the generated CPP1 files and corrects the headers in response to this.

And it's already paying off.

@alexreinking
Copy link
Author

Trying to rebase this on the latest main reveals a syntax error in cpp2util.h, here:

cppfront/include/cpp2util.h

Lines 594 to 596 in 98f6dd4

// A helper for is...
template <class T, class... Ts>
inline constexpr auto is_any = std::disjunction_v<std::is_same<T, Ts>...> {};

The curly braces are not needed.

@alexreinking
Copy link
Author

alexreinking commented Sep 25, 2022

If there were a good way of picking out which tests had a main function and which did not, I would try actually running the binaries (under sanitizers, too) and regression-testing their output.

@JohelEGP
Copy link
Contributor

You can list them explicitly, as with the codegen_only_tests variable. And ensure it's up-to-date by using some tool to inspect the object files for the presence of ::main.

@hsutter
Copy link
Owner

hsutter commented Sep 25, 2022

Trying to rebase this on the latest main reveals a syntax error in cpp2util.h, here:

cppfront/include/cpp2util.h

Lines 594 to 596 in 98f6dd4

// A helper for is...
template <class T, class... Ts>
inline constexpr auto is_any = std::disjunction_v<std::is_same<T, Ts>...> {};

The curly braces are not needed.

Right, gcc flagged that and I thought I fixed that yesterday after the first commit but now I see I didn't commit that change... done, thanks.

@hsutter
Copy link
Owner

hsutter commented Sep 25, 2022

If there were a good way of picking out which tests had a main function and which did not, I would try actually running the binaries (under sanitizers, too) and regression-testing their output.

Good point, I was meaning to clean that up -- now all tests have a main. Let me know if I missed anything. Thanks!

@alexreinking
Copy link
Author

Looks like it's working. I updated the test harness to build executables, but it's not running them yet. That's the next step.

@alexreinking
Copy link
Author

alexreinking commented Sep 25, 2022

@hsutter there's a small handful of tests that don't have a corresponding CPP file because they aren't expected to build.

mixed-initialization-safety-1.cpp2
mixed-initialization-safety-2.cpp2
mixed-lifetime-safety-pointer-init-1.cpp2
mixed-lifetime-safety-pointer-init-2.cpp2
mixed-lifetime-safety-pointer-init-3.cpp2
pure2-bounds-safety-pointer-arithmetic-error.cpp2
pure2-lifetime-safety-pointer-init-1.cpp2
pure2-lifetime-safety-reject-null.cpp2

There's also the one test I'm special casing since cppfront produces a non-buildable cpp1 file from it:

mixed-postfix-expression-custom-formatting

Maybe these could follow a separate naming convention or be moved to a different directory?


Also, can I ask why the cpp2 files are duplicated in test-results?

Copy link

@friendlyanon friendlyanon left a comment

Choose a reason for hiding this comment

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

I haven't tested the code in CppfrontHelpers.cmake locally, but looks like your fork's CI is green.

Besides the lack of documentation, beautiful PR!

@hsutter
Copy link
Owner

hsutter commented Sep 29, 2022

Interim ack: Thanks! I'm potentially convinceable but I'm really trying hard not to complicate things with CMake right now. I fully admit that could be because of my innate aversion to dipping even a toenail into the build system wars (at least yet, at least if I can avoid it). Perhaps for now this could be a separate sister project I link to from the wiki (which I'm about to set up soon)?

there's a small handful of tests that don't have a corresponding CPP file because they aren't expected to build.

I'm working on building these as part of regression testing and will look into these, thanks.

Also, can I ask why the cpp2 files are duplicated in test-results?

No particular reason, it was just simpler to have the parent contain the tests and then copy everything into the working directory. I'm about to push a commit that changes that to remove the duplication (and changes the filename extension for the test output results) as part of getting the regression tests to also execute and to work around Windows short filename legacy weirdness (a gift that keeps on giving in 2022, durnit). I do want to actually run the tests, so I can spot regression errors in cpp2util.h -- thanks for reporting a few already.

More soon, probably this weekend or next weekend (I'm mainly working on this on nights/weekends).

@hsutter hsutter self-assigned this Sep 29, 2022
@alexreinking
Copy link
Author

Interim ack: Thanks!

Thanks for your time looking at this PR!

I'm really trying hard not to complicate things with CMake right now.

I totally understand. My fallback goal with this PR (if it can't be merged) is to at least show what it takes to do CMake right for this project. I think the other attempts so far have fallen far short.

Perhaps for now this could be a separate sister project I link to from the wiki (which I'm about to set up soon)?

I can, of course, re-work this into a sister project which includes cppfront as a git submodule. If we go that route, I do hope that we can work together to get the regression tests in this repository into a state such that I don't have to touch the test harness every time I bump the submodule pointer.

I'm potentially convinceable [...] I do want to actually run the tests, so I can spot regression errors in cpp2util.h -- thanks for reporting a few already.

You're welcome!

Perhaps the test harness I built will overcome other reservations you might have. It already works on both Linux and Windows and can be easily enabled on macOS, once outstanding bugs there are fixed.

If you're worried about ongoing maintenance, I can offer my time addressing issues related to CMake. I can say this with confidence since I will have to do the same things maintaining a sister project, anyway. I have very relevant experience as a maintainer of Halide.

I fully admit that could be because of my innate aversion to dipping even a toenail into the build system wars (at least yet, at least if I can avoid it).

Still, it's worth noting that CMake has been the most popular project model and build system for C++ since 2019. In the last year, even Bryce Adelstein Lelbach has taken to calling CMake the de facto standard build system, noting that it now has majority market share.

For better or worse, if one must choose a single, portable, build system for C++ (and first-party support for multiple is, imho, unsustainable) it should pretty clearly be CMake.

@hsutter
Copy link
Owner

hsutter commented Oct 1, 2022

Thanks @alexreinking, yes please let's set it up as a sister project for now, and that will help me especially in the short term to catch up on the issues being added here and in the medium term implementing the rest of the basic language. Then it will be a great time to look at merging. Thanks again for the interest and expertise, and for understanding!

In parallel over the next couple of weeks I plan to add my own basic execution of the regression tests.

Just let me know here in this comment thread when you have a link to the project, and I'll add it to the cppfront wiki home page. I'll keep this open until then to help me remember.

@hsutter hsutter added enhancement New feature or request suggestion defer and removed enhancement New feature or request suggestion labels Oct 1, 2022
1. Mark custom install dirs as advanced
2. Reduce concurrency in GHA workflow to match runner cores (2)
In builds using FetchContent or add_subdirectory, the cpp2util library
will have its headers marked as SYSTEM to suppress warnings caused by
code in cpp2util.h.

This can be overridden by setting CPPFRONT_NO_SYSTEM to ON.
@alexreinking
Copy link
Author

@hsutter - thanks! That all sounds like a good plan to me.

Here is the link I'd like to use: https://github.com/modern-cmake/cppfront

You can describe it as:

A modern CMake (3.23+) build for cppfront, complete with automatic cpp2-to-cpp1 translation. Compatible with find_package, add_subdirectory, and FetchContent.


I wonder if I could also ask you to please add the missing headers in mixed-captures-in-expressions-and-postconditions:

#include <algorithm>
#include <vector>

@hsutter hsutter closed this in a0d13e1 Oct 4, 2022
@hsutter
Copy link
Owner

hsutter commented Oct 4, 2022

Thanks @alexreinking , I've added the link to the wiki and pushed a commit that adds those headers.

@alexreinking
Copy link
Author

@hsutter - awesome, thanks!

@hsutter hsutter mentioned this pull request Oct 9, 2022
Azmah-Bad pushed a commit to Azmah-Bad/cppfront that referenced this pull request Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants