Reorganize the entire source code tree into modern Java modules #1479
Conversation
|
@rbri and other regular contributors with PRs in the queue -- please take a look at this. I think it's worth it for the long-term evolution of Rhino but it will affect PRs that you have in flight (although git is not bad at figuring out how to merge in these cases). |
|
@gbrail thanks for all the work My hope is still that we start a branch/fork that cuts all the old stuff to be able to move forward to get closer the real world. And therefor i think this can be a good starting point for that... Outside of this i have no real experiences, how much work it will be to migrate the htmlunit-corejs project to stay in sync with this change. So i trust you... |
|
@gbrail From my perspective, moving to a layout that supports Java modules is a good thing. I'm not that well-versed in what that technically entails, so if you say these are the required changes, I'll take your word for it :-) Does this PR fully resolve #1092? As as to @rbri's suggestion to use this as a branch/fork cut-off point that cuts all the old stuff to be able to move forward: imho opinion this restructuring doesn't require a new branch/fork. What's will be the impact for Rhino users/embedders? @rbri If you have specifics on why you want a new branch/form to rid ourselves of old stuff that is hampering progress, could you add to #1363? |
Was reading up on some of the previous discussions about a v2 release that would break backwards compatibility in an effort to shed some historical ballast, which would then allow us to move forward faster @gbrail could you shed some light on if his PR contains any 'breaking' changes for users/embedders? From the description you have, I only get that:
Wrt to minimum Java version moving to 11: see #1169: any impact on rhino usage on Android that we can see? Wrt repo structure change: I assume this only affects contributes or embedders that forked the repo, or? Is there anything more that users/embedders will notice from this change? |
|
Sorry, a few more answers to the questions: Regarding incompatibility -- this PR introduces four distinct and "correct" Java modules that replace "rhino.jar". Projects using "rhino.jar" along with one of these four modules, like "rhino-engine", will likely see errors. So, projects that use Java modules elsewhere will want to stop using "rhino.jar" and use the individual JARs instead. The "rhino.jar" is still around so that people not using modular Java features don't necessarily have to change their build. Yes, this PR will address #1092. I chose not to put fully-qualified package names in the top of the directory tree, however, which means that I'm not sure how to create multi-module Javadoc. Creating a Javadoc for each module is pretty easy though, and I'm sure I can figure it out, and the pathnames are long enough without all that extra stuff IMO. Other issues:
Regarding Android compatibility -- I don't know. In the past the problem has been that we use APIs that are not present in some versions of Android. Without someone who works with Android regularly contributing to this project, I don't know to ensure that we know what this might affect. However, given that I believe this PR is still helpful in other ways, I think we should go ahead. |
|
@gbrail thnx for the response. Sounds like while embedders might have to make a few adjustments how to include rhino in their project, it'll be fairly minor. So I'd day go for it!. If moving to Java 11 means Rhino won't be compatible with older Android version, I think that's just what it is. We can't let backwards compatibility with old platforms hold us back. |
Follow Gradle conventions for a multi-module build. Top level modules: * rhino-runtime: Basic Rhino runtime, enough to run scripts * rhino-tools: Adds Shell, compiler, "Global" object * rhino-xml: Add E4X * rhino: Uber-JAR containing all of the above * rhino-engine: Java ScriptEngine plug-in * tests: Tests, which depend on all above Add proper Java module-info files for each module, and sure that everything works correctly in the new module system.
Use "jmh" Gradle plugin to make it easy to run ./gradlew jmh will now run all the benchmarks.
Update spotbugs and add necessary exclusions Fix one small thing in Context that was making it upset Update other plugins Make spotless work -- currently only runs on Java 11 Set time zone in Mozilla tests Move many tests into modules for better modularity Add publication info for JARs The "rhino" module will include an all-in-one JAR for backward compatibility, but most new code should combine the four modular JARs instead. Enable some tests that work now because of Java 11 minimum Get Gradle to stop trying to download Java versions.
It seems that stack traces are being generated differently and we have no control over that.
Security controllers are no longer supported, so don't test them
|
Nice 👍 |
|
Don't get this part: If the tools don't work on Java 11, why if Java 11 required for regular developers? Do you meant so say that Java 11 is the minimum Java version for users of Rhino, whereas developers working on developing Rhino itself require at least Java ??? To be able to use all the required tooling, like ??? |
|
@gbrail Can it be that there's something wrong in the module setup? Test262SuiteTest.java for me gives an error: I see that class isn't exported in module org.mozilla.rhino.tools If I fix that however, it starts complaining about JsonParserTest importing the non-accessible org.mozilla.javascript.json.JsonParser. Maybe something is wrong in my setup, but I had Gradle set everything up for me (in Eclipse) |
|
Also: I managed to get 2 folders auto-generated: As of yet I'm not sure how they came to be, but if those are part of the building/testing setup, should they maybe be added to .gitignore? |
|
@gbrail Any thoughts on the issues I'm having in tests with the non-accessibility of org.mozilla.javascript.tools.SourceReader and org.mozilla.javascript.json.JsonParser As build on GitLab don't fail, I'm sure it must be something in my setup, but I'm clueless as to what it might be... |
|
Yes, it seems like IDEs are more sensitive to things than gradle and
actually the problems are real problems.
#1505 fixes them!
…On Sat, Jun 29, 2024 at 10:58 AM Paul Bakker ***@***.***> wrote:
@gbrail <https://github.com/gbrail> Any thoughts on the issues I'm having
in tests with the non-accessibility of
org.mozilla.javascript.tools.SourceReader and
org.mozilla.javascript.json.JsonParser
As build on GitLab don't fail, I'm sure it must be something in my setup,
but I'm clueless as to what it might be...
—
Reply to this email directly, view it on GitHub
<#1479 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I2ZVS7K2VBPOM5Y3MPLZJ3DQBAVCNFSM6AAAAABIH62JXOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCOJYGIZDCNJUGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
This PR reorganizes the source tree into proper Java modules. We end up with the following modules:
This PR moves literally every source file. Fortunately, git is pretty good at this -- I have been merging new changes for over a year and rarely get confused. However, if you are working on a large PR and adding many new files you will have to pay attention.
Build instructions:
Benefits:
Side effects:
Opportunities:
Notes:
Please try this out! I think it's a big step forward for maintainability. thanks!