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

Stub Out the Parser Interface #1065

Merged
merged 18 commits into from
Aug 14, 2020
Merged

Stub Out the Parser Interface #1065

merged 18 commits into from
Aug 14, 2020

Conversation

Kesanov
Copy link
Contributor

@Kesanov Kesanov commented Aug 6, 2020

Pull Request Description

Closes #1006

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the Scala, Java, and Rust style guides.
  • All documentation and configuration conforms to the markdown and YAML style guides.
  • All code has been tested where possible.

@Kesanov Kesanov added Type: Enhancement p-highest Should be completed ASAP labels Aug 6, 2020
@Kesanov Kesanov self-assigned this Aug 6, 2020
@Kesanov Kesanov requested a review from wdanilo as a code owner August 6, 2020 07:50
build.sbt Outdated
Comment on lines 793 to 799
lazy val parser = (project in file("lib/scala/parser"))
.settings(
fork := true,
javaOptions += {
val root = baseDirectory.value.getParentFile.getParentFile.getParentFile
s"-Djava.library.path=$root/target/rust/debug"
},
Copy link
Member

Choose a reason for hiding this comment

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

I understand that currently no projects depend on the parser, but in the future, at least runtime will, right?

Is there a plan on how to include these binary dependencies in the distribution of runtime? Will they be packaged in some native_lib directory or inside the JAR? How should users/programs launching the runtime handle the java.library.path?

I feel like this should be documented. I guess it may be added when runtime really becomes dependent on the new parser, but I'm worried we may forget to do so then, so maybe at least a general plan on how this will happen would be good?

Copy link
Contributor

Choose a reason for hiding this comment

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

This should definitely be documented now, both in the parser documentation (the why), and the distribution documentation (the what).

Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

A good start, but it's lacking in documentation.

lib/rust/parser/src/lib.rs Outdated Show resolved Hide resolved
lib/rust/parser/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@iamrecursion iamrecursion left a comment

Choose a reason for hiding this comment

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

Nice improvements, but there's more stuff that needs fixing.

docs/infrastructure/rust.md Outdated Show resolved Hide resolved
docs/infrastructure/rust.md Outdated Show resolved Hide resolved
docs/parser/jvm-object-generation.md Outdated Show resolved Hide resolved
docs/parser/jvm-object-generation.md Outdated Show resolved Hide resolved
lib/rust/parser/src/jni.rs Outdated Show resolved Hide resolved
lib/rust/parser/src/lib.rs Show resolved Hide resolved
project/Cargo.scala Outdated Show resolved Hide resolved
build.sbt Outdated
fork := true,
Cargo.rustVersion := rustVersion,
Compile / compile / compileInputs := (Compile / compile / compileInputs)
.dependsOn(Cargo("build -p parser"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.dependsOn(Cargo("build -p parser"))
.dependsOn(Cargo("build --project parser"))

@Kesanov Kesanov requested a review from radeusgd August 12, 2020 14:05
Copy link
Member

@radeusgd radeusgd left a comment

Choose a reason for hiding this comment

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

Looks really great! I especially like the documentation explaining the Rust infrastructure.

Still it would be cool if these few functions in the project/ directory got at least some short summaries.

Also, I'm not sure if this is in scope in this pull request, but we need to keep in mind that, at some point before starting to use the new parser, we need to:

  • add the native-libraries to the build workflows (both the scala.yml for CI builds and release.yml for releases)
  • update information on compiling the legal information regarding the licences that are used in the dependencies that we distribute.

It may be worth creating separate issues for these, I guess. What do you think @iamrecursion ?

docs/infrastructure/rust.md Outdated Show resolved Hide resolved
project/Cargo.scala Show resolved Hide resolved
@Kesanov Kesanov merged commit 5345bdc into main Aug 14, 2020
@iamrecursion iamrecursion deleted the wip/jv/parser branch August 14, 2020 09:11
@s9ferech s9ferech mentioned this pull request Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p-highest Should be completed ASAP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stub Out the Parser Interface
3 participants