-
Notifications
You must be signed in to change notification settings - Fork 324
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
Reworking Excel support to allow for reading of big files #8403
Changes from 72 commits
c8e2b40
8a34596
c6d8a37
cd26b95
e5c1bf0
3003bbd
5a9929f
11be28e
13d4009
9a4e4c0
02e76c2
ad8b60d
db2b4e3
17e2ab4
3f35168
fdf4f41
deed106
1e8502f
408d5bf
6533c38
cf5267b
72f9517
38e957a
9bf9b14
4b80e9f
b1d6a92
464a3f6
d313996
7e739d8
a5e5866
9590b6a
0208dd4
8da2715
fc850d5
882fb19
178caf0
9ecb477
18cdcce
f62b36d
58647b0
fc12dce
0bcf194
240f1a0
e989643
bd7fd96
b604286
5eb516d
fa26641
3b241b2
d80ce7c
22717b2
1fcc33c
fa0faf1
37d92d2
b12bf59
cf5d758
29b5caa
b8169cd
8853469
c5cd63f
0f45573
a147278
3b1ea01
d5d83a1
15ecad5
9a22d3a
ddc3b2e
c5f99c9
4b5e589
f00d2d3
e427758
fa11eab
10ab3e1
a4ec0e0
3ea7805
858b1b4
0747c08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1330,13 +1330,16 @@ lazy val truffleDslSuppressWarnsSetting = Seq( | |
) | ||
|
||
/** A setting to replace javac with Frgaal compiler, allowing to use latest Java features in the code | ||
* and still compile down to JDK 11 | ||
* and still compile down to JDK 17 | ||
*/ | ||
lazy val frgaalJavaCompilerSetting = Seq( | ||
lazy val frgaalJavaCompilerSetting = | ||
customFrgaalJavaCompilerSettings(targetJavaVersion) | ||
|
||
def customFrgaalJavaCompilerSettings(targetJdk: String) = Seq( | ||
Compile / compile / compilers := FrgaalJavaCompiler.compilers( | ||
(Compile / dependencyClasspath).value, | ||
compilers.value, | ||
targetJavaVersion | ||
targetJdk | ||
), | ||
// This dependency is needed only so that developers don't download Frgaal manually. | ||
// Sadly it cannot be placed under plugins either because meta dependencies are not easily | ||
|
@@ -2731,11 +2734,16 @@ val allStdBits: Parser[String] = | |
lazy val `simple-httpbin` = project | ||
.in(file("tools") / "simple-httpbin") | ||
.settings( | ||
frgaalJavaCompilerSetting, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The frgaal compiler settings have been causing compile error: Apparently Anyway there is no benefit in Frgaal within this sub-project, especially as we are already on JDK21. Removing it allowed me to re-use the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Frgaal by default only exposes only classes that are in API of the JDK.
There is a benefit. It already prevented you from using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I really honestly don't understand. HOW is that a benefit? Without Frgaal the server is running fine on our JDK. What is the benefit of using it this translation layer at this point? We introduced it to be able to use newer features before we migrated to the newer JDKs. But now we have these features natively, so what else? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You have compiled your code in a way that allows running it on JDK 11. However your code cannot run on JDK 11. Without Frgaal you wouldn't even notice that your project is misconfigured as it requires JDK18+ to run. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hm, I see. It was hard to appreciate that, because from my perspective, removing Frgaal made it work - I never really specifically wanted to compile |
||
customFrgaalJavaCompilerSettings(targetJdk = "21"), | ||
autoScalaLibrary := false, | ||
Compile / javacOptions ++= Seq("-Xlint:all"), | ||
Compile / run / mainClass := Some("org.enso.shttp.SimpleHTTPBin"), | ||
assembly / mainClass := (Compile / run / mainClass).value, | ||
libraryDependencies ++= Seq( | ||
"org.apache.commons" % "commons-text" % commonsTextVersion | ||
) | ||
), | ||
(Compile / run / fork) := true, | ||
(Compile / run / connectInput) := true | ||
) | ||
.configs(Test) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we change the
-target
level to17
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is any sophisticated reason for that. I just thought that it was a good idea to bump that to
-target 17
. Furthermore, I vaguely remember that we talked about this as well. But I think we can fall back to 11.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have only updated the documentation comment to be aligned with what is actually being done.