-
Notifications
You must be signed in to change notification settings - Fork 0
Analyze, document, and reduce org.jnode.vm.core ↔ rt circular dependency at source level #30
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
base: mavenizer
Are you sure you want to change the base?
Conversation
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
|
@copilot apply "Phase 1" |
- Moved core/src/core/org/jnode/permission/JNodePermission.java to core/src/classpath/vm/org/jnode/permission/JNodePermission.java - Added org.jnode.permission.* export to rt plugin descriptor - Removed org.jnode.permission.* export from vm.core plugin descriptor - Reduced circular dependency from 154 to 153 classes Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
Phase 1 complete! ✅ Moved JNodePermission from org.jnode.vm.core to rt plugin as proposed. Results:
Changes in commit 9290402:
The analysis tool confirms the change is working correctly. JNodePermission now lives in rt where it belongs (only depends on java.security.BasicPermission). |
|
@copilot I still see the circular dependency between org.jnode.vm.core and rt by running analyze_source_cycles.py |
You're correct - the plugin-level circular dependency still exists. What Phase 1 achieved:
Why the cycle still exists:
The reality:
This is a classic bootstrap coupling that cannot be fully eliminated without major architectural changes. Phase 1 was about reducing accidental coupling (JNodePermission). The remaining ~153 classes represent intentional architectural coupling. The proposal's recommendation is to accept most of this coupling as by-design and focus only on the few accidental dependencies (like JNodePermission) that can be reasonably moved. |
Co-authored-by: fduminy <143904+fduminy@users.noreply.github.com>
|
@copilot how can I deal with this circular dependencies if i convert each jnode plugin to a maven module ? |
Maven doesn't allow circular dependencies between modules - the build will fail. Here are your options: Option 1: Merge Circular Modules (Recommended for vm.core ↔ rt)
Option 2: Extract Common API Module
Option 3: Use Maven's <dependency>
<groupId>org.jnode</groupId>
<artifactId>jnode-vm-core</artifactId>
<scope>provided</scope>
</dependency>This won't break Maven's build order but doesn't truly eliminate the cycle. Practical Recommendation:
The mavenizer tool in the repository may need updates to handle this merged module approach. |
|
@copilot If I name TOTO the merge of org.jnode.vm.core and rt,
|
Correct observation! After merging vm.core + rt into TOTO, you'd still have these cycles: Small cycles with TOTO:
Larger cycle: Maven strategy: A) Merge more aggressively (simplest):
B) Keep some separation (more complex):
Option A is recommended for Maven conversion - these 5 plugins form the bootstrap core and are fundamentally coupled. |
Analyzed the circular dependency between
org.jnode.vm.coreandrtplugins at Java source level. Identified 154 classes involved, sorted by dependency count, and proposed concrete refactoring strategies. Implemented Phase 1, reducing the circular dependency from 154 to 153 classes.Analysis Tool
analyze_vm_core_rt_cycle.py- Automated dependency analyzer that:python3 analyze_vm_core_rt_cycle.py # Output: VM_CORE_RT_CYCLE_DETAILED_ANALYSIS.mdKey Findings
153 classes in cycle (reduced from 154):
Top refactoring candidates (1 dependency each):
✅ Moved to rt,JNodePermissionVmSystemSettings,BaseVmArchitecturejavax.isolate.Isolate,javax.isolate.Link,javax.isolate.StreamBindingsImplementation Status
Phase 1 - Quick win: ✅ COMPLETE - Moved
JNodePermissionfrom vm.core to rtcore/src/core/org/jnode/permission/JNodePermission.javatocore/src/classpath/vm/org/jnode/permission/JNodePermission.javaorg.jnode.permission.*org.jnode.permission.*exportPhase 2 - Isolate API: Extract
org.jnode.vm.isolate.apiplugin with interfaces only, breaking javax.isolate ↔ VmIsolate cycle (3-5 days)Phase 3 - Accept design: 31/41 rt→vm.core dependencies are Native* implementations (NativeObject, NativeSystem, etc.). This coupling is fundamental to JNode's bootstrap architecture - document rather than eliminate.
Architecture Insight
The VM and runtime are intentionally coupled through native method implementations. Full cycle elimination is neither necessary nor beneficial. Focus: reduce accidental coupling, document intentional coupling.
Documentation
VM_CORE_RT_CYCLE_DETAILED_ANALYSIS.md- Complete 153-class breakdown sorted by dependency count (updated)BREAKING_VM_CORE_RT_CYCLE_PROPOSAL.md- 3-phase implementation plan with risk assessmentCIRCULAR_DEPENDENCY_README.md- Usage guide and quick referenceCIRCULAR_DEPENDENCY_SUMMARY.md- Executive summaryOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.