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

Redesign of the repository layout and gradle configuration #1779

Merged
merged 106 commits into from
Jun 2, 2023
Merged

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented May 24, 2023

Overview and Motivation

This PR presents a major refactoring of the source layout in our repository as well as the gradle configuration that we use for building. Our current layout was created by Xtext and does not follow the Gradle/Maven layout conventions that are typical for Java projects (#1801). Moreover, our build configuration (gradle) grew over time, without ever considering the idiomatic gradle way of solving certain problems. This lead to a situation where we don't support gradle's standard tasks like build and assemble (#1514). Moreover, there were several issues with the setup like Kotlin classes missing on a rebuild (#1284) and whenever we tried to modify something in the configuration something else broke. For this reason, this PR rewrites the gradle scripts from scratch using an idiomatic approach.

Goals

Changes

Project structure

The new project structure looks like this:

org.lflang
├── cli
│   ├── base
│   │   ├── build.gradle
│   │   └── src/
│   ├── lfc
│   │   ├── build.gradle
│   │   └── src/
│   ├── lff
│   │   ├── build.gradle
│   │   └── src/
├── core
│   ├── build.gradle
│   └── src/
└── lsp
    ├── build.gradle
    └── src/

In the root of the repository (the root project) we have one sub-project org.lflang that is divided into multiple nested sub-projects. This is in contrast to the single project that we had before. Gradle doesn't like if certain plugins are combined (e.g. library and application) and instead one should be explicit about which sub-projects are libraries (with public API or internal) and which are executable applications.

In the new setup, we have the core projects which is a library and holds the largest part of our code base. The lsp project contains is application and holds sources that are specific to the language server. The cli/lfc and cli/lfc are both applications and of course correspond to our cli tools. cli/base is a library containing logic that is shared between the two.

Source Layout

Each of the src/ directories follows the Gradle/Maven source tree layout and looks something like this:

src
├── integrationTest
│   └── java
├── main
│   ├── java
│   ├── kotlin
│   └── resources
├── test
│   └── java
└── testFixtures
    └── java

Note that we divide the source tree by function (main i.e main sources, test, testFixtures and integrationTest) and then by language (java, kotlin, resources). resources contains all additional resources that should be on the classpath like our runtime submodules.

Testing

Instead of having a separate test project (the org.lflang.tests from before) we now make the tests part of each project (i.e. we place the tests close to the code they are testing). In the source layout from above, the test subdirectory contains all unit tests. We make use or gradle's test suite feature and also define an additional test suite integrationTest. These are our runtime tests that compile and execute each test LF program. Separating unit and integration tests gives us more control about which types of tests we run. The test task will only run all unit tests and not the slow integration tests.

The integration tests can be run using ./gradlew integrationTest. We can also specify the precise test as before. For instance, the following command will run all concurrency tests for the C target:

./gradlew org.lflang:core:integrationTest --tests org.lflang.tests.runtime.CTest.runConcurrentTests

Note that we qualify the integrationTest task with the project, so that we only search for matching tests in this project.

The root project defines a convenient helper tasks that allows to run all tests for a specific target. For instance, you can use the following command to run all Rust tests:

./gradlew targetTest -Ptarget=Rust

You can specify any valid target. If you run the task without specifying the target property ./gradlew tagetTest it will produce an error message and list all available targets.

Another root task allows running only a single specific test:

./gradlew singleTest -DsingleTest=test/C/src/Minimal.lf

Note that this replaces the runSingleTest task.

To invoke only the C tests in the concurrent category, for example, do this:

./gradlew org.lflang:core:integrationTest --tests org.lflang.tests.runtime.CTest.runConcurrentTests

Note that the description above can also be found in the updated test/README.md.

Building and running with Gradle

To build everything, simply run ./gradlew build. Note that the build task in Gradle also includes running all checks. So build will also execute the test and checkFormat tasks. If you don't want the checks to run, ./gradelw assemble can be used instead. However, I think it is great to always make sure that the checks pass when building and Gradle is quite smart in figuring out which checks to run. It will only rerun those affected by a change. Overall, build should be quite fast.

Both the build and the assemble task will create executable lfc and lff scripts in build/install/lf-cli/bin.

As before, we also define custom tasks runLfc and run runLff that can be used to directly execute the cli applications:

./gradlew runLfc --args=test/C/src/Minimal.lf

Packaging

Packaging is trivial now. Gradle's distribution plugin takes care of it. It creates zip and tar archives in build/distributions that are ready to be uploaded as artifacts. In fact, the installed scripts from above are the unpacked version of these archives.

For nightly builds, the nightly project property can be specified. For instance:

./gradlew runLfc -Pnightly

This will create an archive called something like lf-cli-0.4.1-SNAPSHOT-nightly-20230531150822.zip. Note that the name now also includes the snapshot version, as before it was named lf-cli-nightly-20230531150822.zip. This shouldn't be an issue, though.

Scripts in bin

We have collected several bash scripts in bin/, many of which have rarely been used or are redundant.

  • measure-lf-time and run-multiple-times are simply not needed as this functionality is covered by the benchmark runner.
  • build-lf-cli and run-lf-tests are also not needed as we can instead invoke the corresponding gradle task directly.
  • bump_versions we do need, but I moved it to util/scripts/bump-versions. It should only be used rarely and should probably not be located in the prominent bin directory.
  • lfc and lff see below

bin/lfc and bin/lff scripts

I decided to make the bin/lfc and bin/lff scripts as simple as possible. They simply call the corresponding gradle run tasks. Running ./bin/lfc <arg1> <arg2> <...> is equivalent to running ./gradlew runLfc --args="<arg1> <arg2> <...>".

Advantages:

  • The scripts are as simple as it gets. We don't have to check if the code was actually build and don't have to produce an error message if the user needs to build first.
  • Gradle will make sure that everything is build up to date. So lfc and lff will always run using the latest code, which is what we (at least I ;) ) want in a developer setup.
  • Incremental builds are typically very fast for the run tasks (gradle will only do what is necessary for running).
  • Tab-Completion on the command line works when using the scripts but not when using the gradle run tasks (at least for me)

Disadvantages:

  • Gradle will produce output before and after the actual output from lfc/lff.

Note that users and developers can still use the lfc/lff scripts in build/install/lf-cli/bin if they prefer. Those invoke the commands directly (not through gradle).

CI Flows

Since the build task includes running unit tests and formatter checks, we don't need separate CI jobs for the latter two anymore.

Idea Plugin

I also added the idea plugin to our root project. This allows to run ./gradlew openIdea to open the project in IntelliJ IDEA.

Open questions

  • Should we eliminate the org.lflang directory and have cli, core, lsp directly as sub-projects of the root project?
  • Is changing the behavior of bin/lfc and bin/lff controversial or is it Ok for everyone?

TODO

  • Figure out why ominous directory tmp/fed-gen is created in project root after running unit tests.
  • Move util/scripts/bump-versions to epock repo (still works and fix if needed. It should also consider the versions in
  • The release tooling needs to be updated ( after merging this)
  • There is a file in org.lflang/todo/log4j.xml where it is unclear what it was needed for (the current location in master is org.lflang/todo/log4j.xml). Was this needed for the LSP or for the CLI tools? Either way, it should be added as a resource to the corresponding application projects.
  • Add dummy tasks for our old tasks (buildAll, runSingleTest) with an informative error message indicating what to do instead.
  • Update benchmark CI tests.
  • The paths used to check which CI tests need to run need to be updated. (@petervdonovan)
  • Update documentation on the website (Update the developer documentation lf-lang.github.io#140)
  • Test the LSP distribution.
  • Integrate changes in epoch repo. -> do later

Fixed issues

@cmnrd cmnrd added the gradle Issues regarding Gradle build configuration label May 24, 2023
@cmnrd cmnrd requested review from lhstrh and petervdonovan May 24, 2023 17:11
@cmnrd cmnrd mentioned this pull request May 26, 2023
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 1, 2023

@petervdonovan I added a the shadow jar plugin back to the lsp supbroject. Now ./gradlew lsp:assemble or ./gradlew lsp:build will also create a fat jar. This can be found in lsp/build/libs/lsp-0.4.1-SNAPSHOT-all.jar. I assume that the VS code extension build needs to be changed, as the paths changed. Could you take care of updating the extension and testing if the lsp jar that we produce works as before?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 1, 2023

@erlingrj Do you have any idea why the zephyr tests are failing?

@erlingrj
Copy link
Collaborator

erlingrj commented Jun 1, 2023

I think the problem is that my RunZephyrTests.sh script requires that all the directories below test/C/src-gen are built for Zephyr. It recursively executes each test on the QEMU emulator. There seems to be some leftover src-gen directories, from previous workflows here. Can I pass an argument to gradlew inorder to pass -c to lfc and delete all the previous src-gen directories?

If they are based on RuntimeTest, they will inherit all the standard test
methods as well.
@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 1, 2023

Ok, that gave me the right pointer. I fixed it in 75a0578 (hopefully). The problem was that the new CI setups runs all test of the CZephyrTests class. Since that was based on RuntimeTest it also inherited all the default C tests.

@petervdonovan
Copy link
Collaborator

Because Christian added back a way to construct a fat jar for the language server, it is trivial to update vscode-lingua-franca to be compatible with these changes (lf-lang/vscode-lingua-franca#120). In any case, we don't have to update vscode-lingua-franca right away since it still tracks the same commit as a submodule, so that should not block our merging this.

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.

I am far from expert in Gradle, but I think this is probably ready to merge.

Thanks Christian!

Comment on lines -158 to +154
.replace(lfFile.toString(), "%%%PATH.lf%%%")
.replace("%%%PATH.lf%%%", lfFile.toString())
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happened here? It seems like the original intent was to replace the value of lfFile.toString() with "%%%PATH.lf%%%" and not the other way around.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not entirely sure either. The test used to fail and worked fine after switching this.

@petervdonovan
Copy link
Collaborator

In order to get this to work in IntelliJ, I needed to go to Help > Edit Custom VM Options and add the following to the bottom of the .vmoptions file:

-DsingleTest=test/C/src/Minimal.lf
-Ptarget=C

We should make sure people know about this to save them some trouble.

@lhstrh
Copy link
Member

lhstrh commented Jun 2, 2023

In order to get this to work in IntelliJ, I needed to go to Help > Edit Custom VM Options (...)

@cmnrd, do you think there is a way to avoid this extra step?

@cmnrd
Copy link
Collaborator Author

cmnrd commented Jun 2, 2023

I am a bit confused by this. What exactly did not work for you? Actually singleTest and target are not supposed to be set at the same time.

Normally, the way to configure the integrationTest or singleTest tasks would be to create a run configuration for them. This is also discussed in our documentation.

Or was your problem that IntelliJ would throw errors when the tasks are loaded initially? If that was the case for you, it should be fixed by b2c70d2. This ensures that the errors are only thrown when the task actually executes, not when it is loaded.

@cmnrd cmnrd enabled auto-merge June 2, 2023 14:29
@cmnrd cmnrd merged commit f8fcaf8 into master Jun 2, 2023
@cmnrd cmnrd deleted the gradle branch June 2, 2023 16:46
cmnrd added a commit to lf-lang/benchmarks-lingua-franca that referenced this pull request Jun 2, 2023
@petervdonovan
Copy link
Collaborator

Or was your problem that IntelliJ would throw errors when the tasks are loaded initially? If that was the case for you, it should be fixed by b2c70d2.

You are right, the problem seems to be solved now!

@petervdonovan petervdonovan added the refactoring Code quality enhancement label Aug 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
gradle Issues regarding Gradle build configuration refactoring Code quality enhancement
Projects
None yet
5 participants