Skip to content

Added Build System Integration #4

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

Closed
wants to merge 18 commits into from
Closed

Conversation

Eshanatnight
Copy link

@Eshanatnight Eshanatnight commented Sep 19, 2022

Added Cmakelists.txt, .clang-format, .clang-tidy and premake5.lua .

Because of this .gitignore file was modified to not include the build directory.
and a .gitattributes file was added

@Eshanatnight Eshanatnight changed the title Added BuildSystem Integration Added Build System Integration Sep 19, 2022
@Eshanatnight
Copy link
Author

Eshanatnight commented Sep 19, 2022

forgot to push premake5.lua when the first comment was made :>

@Frityet
Copy link

Frityet commented Sep 20, 2022

Why premake?

@Eshanatnight
Copy link
Author

Actually i also wrote a cmakelists.txt but for some reason it got deleted?

And as for why premake, after no specific reason I started using premake instead of cmake because i wanted to learn Lua and i thought the fastest way would be to use it

@Eshanatnight
Copy link
Author

Just added the Cmakelists file. Found why it was not being tracked by git, .txt files were in the .gitignore

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.

I find this unsatisfactory because the executable isn't exported. I can only use this by add_subdirectory and not find_package.

@Eshanatnight
Copy link
Author

Eshanatnight commented Sep 20, 2022

Need to find a way to make the tests

I find this unsatisfactory because the executable isn't exported. I can only use this by add_subdirectory and not find_package.

Then would we not need to make the tests in to a "sub-projects" of sorts? Rather than the current way of using .bat files?

@JohelEGP
Copy link
Contributor

My comment is not in regard to tests, but to consumers of the cppfront executable.

@Eshanatnight
Copy link
Author

Eshanatnight commented Sep 20, 2022

My comment is not in regard to tests, but to consumers of the cppfront executable.

Oh I understand you meant that the CMakeLists File is unsatisfactory. The executable can easily be copied/moved with a post build command. To a standard location. This should also help in making and running the tests easier, in my opinion.

And I left the Output Dir relative to the build system as the Author & Maintainers might want something Specific

.gitignore Outdated
Comment on lines 1 to 2
/.vs
/.vscode

Choose a reason for hiding this comment

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

Suggested change
/.vs
/.vscode

I feel like adding editor specific folders to the .gitignore is a bad rabbit hole, you end up adding them for every IDE. This belongs in a user level .gitignore instead.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah you are right. I'll change it then.

@Eshanatnight
Copy link
Author

I would like to ask for some suggestions regarding the tests. Should I classify them as a sub-project to cppfront. I tried running it via the add_custom_command but that is resulting in either by failing to find the files or giving a bunch of errors.

@Colliflower
Copy link

I think it might be better to split some of the content from this PR into multiple PRs btw. There are a few things that seem to be unrelated to build systems such as the .md file changes, clang-format and clang-tidy. Those getting their own PRs would clean things up and make this PR more attractive imo.

@Eshanatnight
Copy link
Author

I apologise, my brain gets all over the place when I work, so it got a bit out of hand it that way.

What i thought of about the testing, is that it's better if we just keep the Tests as they are atleast for now.

@Eshanatnight
Copy link
Author

The CMakeLists is complete, but I am keeping The premake5.lua file as imo it creates a better visual studio sln file than cmake.

@filipsajdak
Copy link
Contributor

@Eshanatnight your PR is a collection of unrelated changes (adding build system files, minor edits in Markdown files) - consider moving them to separate PRs. It will make reviewing them easier.

@Eshanatnight
Copy link
Author

@Eshanatnight your PR is a collection of unrelated changes (adding build system files, minor edits in Markdown files) - consider moving them to separate PRs. It will make reviewing them easier.

I apologize, I will remove the misc files and edits and remove the premake file

@Eshanatnight
Copy link
Author

I have made the asked changes.
Again apologies for the mis steps.

@Eshanatnight Eshanatnight marked this pull request as ready for review September 23, 2022 07:41
@Arthapz
Copy link

Arthapz commented Sep 23, 2022

CMakelists.txt Outdated
)

add_executable(${PROJECT_NAME} ${SRC_FILES})
target_include_directories(${PROJECT_NAME} PUBLIC "include")
Copy link
Contributor

Choose a reason for hiding this comment

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

Compiling cppfront doesn't require this.

Copy link
Author

Choose a reason for hiding this comment

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

Can you please clarify what is not required?

Copy link
Contributor

Choose a reason for hiding this comment

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

Line 20. You can compile cppfront without cpp2util.h.

@hsutter
Copy link
Owner

hsutter commented Sep 24, 2022

Thanks for this idea and energy.

How about making this a separate companion project, and I can add a wiki page of links to known companion projects to help make people aware of them?

My concern about putting it here is that it complicates what I will need to maintain, particularly at this early stage when a lot of things are still fluid and subject to change. And while I realize CMake is usually the most popular and it's a meta-build system that can generate others, if we add CMake I would expect to get questions like "why not Ninja, msbuild, Xcode, Bazel, Gradle, ...?" which I'd have to answer.

Right now I've been considering "no CMake/Bazel/..." to be a feature, in that the project is simple enough that it's easy for anyone to separately build and integrate with whatever build tools and project system they're using. Is that reasonable for now?

@JohelEGP
Copy link
Contributor

JohelEGP commented Sep 24, 2022

Yes, please. I'd like to add https://github.com/JohelEGP/jegp.cmake_modules/#jegpcpp2 to the wiki, which allows to compile CMake targets with Cpp2 source files. You can see it working on CE: https://godbolt.org/z/e1r3qsE8M.

Comment on lines +4 to +5
set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)
set(CMAKE_RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/bin)

Choose a reason for hiding this comment

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

This is super unfriendly to FetchContent users and totally unnecessary.

Copy link
Author

Choose a reason for hiding this comment

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

I am using MSVC cmd line. And the cmd line tool has a tendency to make to obfuscate the build dir imo thats why I added that.

@@ -0,0 +1,19 @@
cmake_minimum_required(VERSION 3.2)

Choose a reason for hiding this comment

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

CMake 3.2 predates even C++17, so this minimum is bogus.

Copy link
Author

Choose a reason for hiding this comment

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

I am sorry I had no idea that was the case. Thanks For Letting me know.

@hsutter hsutter self-assigned this Oct 1, 2022
@hsutter
Copy link
Owner

hsutter commented Oct 1, 2022

Yes, please. I'd like to add https://github.com/JohelEGP/jegp.cmake_modules/#jegpcpp2 to the wiki, which allows to compile CMake targets with Cpp2 source files. You can see it working on CE: https://godbolt.org/z/e1r3qsE8M.

Done, thanks. Added to wiki home page

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.

8 participants