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

Add reactor-cpp as a submodule and reorganize the C++ build process #971

Merged
merged 31 commits into from
Feb 28, 2022

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Feb 17, 2022

This PR brings the following changes:

  1. Make reactor-cpp a submodule of LF.
  2. Overhaul the C++ build process (This has become necessary to support 1. while still allowing mixing of multiple reactor-cpp versions)
  3. Make C++ code generation smarter by only overwriting files if they actually change. This enables cmake to properly detect that it does not need to rebuild a file and significantly speeds up compilation times after making changes to an LF file.

The new C++ build process

Consider this LF project structure:

src/
    A.lf
    B.lf

Where A.lf and B.lf both contain a main reactor.

Previously, compiling A.lf would create a completely isolated cmake project in src-gen/A/. With this PR, this structure is created instead:

src-gen/
    A/
        CMakeLists.txt
        <source files>
    CMakeLists.txt
    reactor-cpp-lfbuiltin

The internals of src-gen/A/ are similar to before. However, we get a new top-level CMakeLists.txt that creates a top-level cmake project for the entire LF project. A/ is automatically included as a subproject. The directory reactor-cpp-lfbuiltin is automatically copied from the reactor-cpp submodule. The -lfbuiltin suffix indicates that it is the built-in reactor-cpp version of LF.

Let's assume we now compile B.lf and it sets this target property: runtime-version: foo. This will modify the src-gen directory as follows:

src-gen/
    A/
        CMakeLists.txt
        <source files>
    B/
        CMakeLists.txt
        <source files>
    CMakeLists.txt
    reactor-cpp-foo
    reactor-cpp-lfbuiltin

The reactor-cpp-foo directory will be automatically fetched via git. The sub-project B is configured to depend on reactor-cpp-foo, while A still depends on reactor-cpp-lfbuiltin.

Overall, this change makes the build-process a lot more robust. CMake is aware of all the files within a project and knows exatly what needs to be compiled when.

I am not sure if these changes might interfere with the LSP linting. @petervdonovan could you check?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 17, 2022

While working on this, I noticed that many methods in FileConfig should better be factored out in a FileUtils class, as they only provide file system access without depending on any configuration. I will make this change in a separate PR as to avoid hiding the changes made in this PR.

@petervdonovan
Copy link
Collaborator

This sounds like a big improvement! Cloning the Git repository has definitely been a slow point in the language server. From your description it sounds like it probably will break CppValidator since the CppValidator depends on variables in CMakeCache.txt. I will take a look as soon as I can.

Copy link
Collaborator

@petervdonovan petervdonovan left a comment

Choose a reason for hiding this comment

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

With the changes I just pushed, the language server seems to work -- I checked by merging this into the lsp branch locally. LSP tests also pass for me locally, so hopefully there are no more blockers on that front.

org.lflang/src/org/lflang/FileConfig.java Outdated Show resolved Hide resolved
org.lflang/src/org/lflang/FileConfig.java Show resolved Hide resolved
org.lflang/src/org/lflang/FileConfig.java Outdated Show resolved Hide resolved
@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 23, 2022

Thanks for your fixes! I will clean things up then and get it to work in Epoch.

@cmnrd cmnrd added this to the 0.1.0 milestone Feb 25, 2022
@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 25, 2022

As far as I am concerned, this is ready to be merged. The copy mechanism now also works from Epoch and was (manually) tested. It would be great if we could include this PR in the 0.1.0 release. I would only need an approval ;)

@cmnrd cmnrd mentioned this pull request Feb 25, 2022
Copy link
Member

@lhstrh lhstrh left a comment

Choose a reason for hiding this comment

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

Look very good! Would the directory name reactor-cpp-default maybe be better than reactor-cpp-lfbuiltin? The latter looks a bit odd and difficult to read, IMO.

@cmnrd
Copy link
Collaborator Author

cmnrd commented Feb 26, 2022

Yes, probably default is better

@cmnrd cmnrd merged commit 899c325 into master Feb 28, 2022
@cmnrd cmnrd deleted the cpp-submodule branch February 28, 2022 07:14
@lhstrh lhstrh added the refactoring Code quality enhancement label Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Code quality enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants