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

Towards simplifying runtime-compiler dependencies #8894

Merged
merged 17 commits into from
Apr 25, 2024

Conversation

JaroslavTulach
Copy link
Member

@JaroslavTulach JaroslavTulach commented Jan 30, 2024

Pull Request Description

Initial step to address #8888. Splits polyglot-api (which has inherent dependencies on Jackson) into two modules. One of them is called engine-common and has no dependencies on external libraries. Dependencies of runtime* projects changed to engine-common.

Not Part of this PR

The suggestion builder functionality has to go away from engine/runtime. The idea is to create a new project called runtime-suggestions in one of the subsequent PRs. Such a change is left out from the PR to keep it smaller, and to minimize conflicts during merging. There is serialization manager issue to be solved.

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • All code follows the
    Scala,
    Java,
  • All code has been tested:
    • Get all the tests running again.

@JaroslavTulach JaroslavTulach added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 30, 2024
@JaroslavTulach JaroslavTulach self-assigned this Jan 30, 2024
@JaroslavTulach
Copy link
Member Author

JaroslavTulach commented Jan 30, 2024

We need to solve the serialization manager issue. Thinking about it, it looks like the best way to solve it is to rewrite caching as requested by

and remember that a single SerializationManager is probably sub-optimal when requests for caching come from two different modules - runtime and runtime-suggestions.

build.sbt Outdated
.dependsOn(`runtime-instrument-repl-debugger`)
.dependsOn(`runtime-instrument-runtime-server`)
.dependsOn(`runtime-language-epb`)
.dependsOn(LocalProject("runtime-instrument-common"))
Copy link
Member

Choose a reason for hiding this comment

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

Originally, I have rewritten runtime to LocalProject("runtime"), because sbt was giving me some error. Is this a similar case? If not, let's rather stick with the backtick notation - that gives me navigation capabilities in my IDE.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is the same case. A cyclic reference between lazy val projects.

I tend to believe we should move integration tests from runtime to runtime-fat-jar project and avoid these vicious circles of project dependencies...

@JaroslavTulach JaroslavTulach changed the title Avoid runtime-compiler dependencies on Jackson Towards avoid runtime-compiler dependencies on Jackson Apr 24, 2024
@JaroslavTulach JaroslavTulach added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Apr 25, 2024
@JaroslavTulach JaroslavTulach changed the title Towards avoid runtime-compiler dependencies on Jackson Towards avoiding runtime-compiler dependencies on Jackson Apr 25, 2024
@JaroslavTulach JaroslavTulach changed the title Towards avoiding runtime-compiler dependencies on Jackson Towards simplifying runtime-compiler dependencies Apr 25, 2024
@JaroslavTulach JaroslavTulach merged commit 931baa4 into develop Apr 25, 2024
36 checks passed
@JaroslavTulach JaroslavTulach deleted the wip/jtulach/NoCompileTimeJackson branch April 25, 2024 08:03
@@ -578,9 +578,12 @@ private class DefaultPackageRepository(
val cache = ensurePackageIsLoaded(libraryName).toOption.flatMap { _ =>
if (!loadedLibraryBindings.contains(libraryName)) {
loadedPackages.get(libraryName).flatten.foreach(loadDependencies(_))
val cachedBindingOption = context
val cachedBindingOption = None
/* TBD: this has to be called somehow
Copy link
Member Author

Choose a reason for hiding this comment

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

@hubertp discovered this commented out piece of code. Thank you!

@JaroslavTulach JaroslavTulach mentioned this pull request Jun 7, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants