Give feature-private implementations feature-owned packages - #79
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
Two Kotlin package roots were each declared by more than one Gradle module:
data.local— by:data:local,:data:gitand:feature:terminaldata.buildsystem— by:data:buildand:feature:buildrunA split package hides real coupling. Files in different modules appear to be in the same package, so they reference each other with no import at all, and no tooling that reasons about imports can tell which module owns a symbol. It also makes package names actively misleading:
:feature:terminalowned files calleddata.local.pty.*, and:feature:buildrunowneddata.buildsystem.install.*.This bit during the earlier dependency audit, where a package-level search could not distinguish a
:data:gitsymbol from a:data:localone and produced the wrong answer.What changed
Feature-private implementations now live under the package of the module that owns them:
data.local.pty.*→feature.terminal.pty.*data.local.ShellTerminalRepository→feature.terminal.ShellTerminalRepositorycore.linux.*→feature.terminal.linux.*data.buildsystem.install.*→feature.buildrun.install.*data.onboarding.*→feature.onboarding.data.*:data:gitmoved offdata.localonto its owndata.gitroot:appno longer declares components implemented by:feature:buildrun.InstallConfirmActivity,InstallStatusReceiverandRemoteBuildKeepAliveServiceare declared in the buildrun manifest and reach the app through manifest merging.Nothing moved between modules. Every file stayed in the module it was already in; only its package changed.
The terminal rename is not a Kotlin-only refactor
The PTY package name is written down in three places that must agree, and only one of them is Kotlin:
NativePtyobjectapp/src/main/cpp/asl_pty.c, which encode the package into the symbolapp/src/main/keepRules/rules.keepA mismatch does not fail the build. It fails at runtime with
UnsatisfiedLinkErrorthe first time a terminal is opened, or only in a release build if the keep rule stops matching and R8 strips the native methods. All three were changed together.Behavior
No behavior change. Package declarations, imports, JNI symbol names, one keep rule, and manifest ownership.
One latent problem surfaced and was fixed:
JGitGitRepositoryusedFileChangeBusandDefaultWorkspaceWriteGatefrom:data:localwith no import, which only compiled because the split package made them look local. They are now explicit imports. That is exactly the coupling a split package conceals.Scope
:data:gitand:data:localremain separate modules with a real dependency between them; only the package overlap is gone.Tests
NativePtyBindingTestin:appties the JNI symbols inasl_pty.cto the class named by the R8 keep rule, and asserts every declared native method has an implementation. A future package rename that updates Kotlin but forgets the C or the keep rule now fails a test instead of a device.Verification
From a clean worktree at the branch tip:
./gradlew test detekt verifyModuleBoundaries :app:assembleDebug— BUILD SUCCESSFULTwo checks the build alone does not perform:
JNI symbols were read back out of the compiled library.
libasl_pty.soexports exactly the four expected symbols under the new package, matching the fourexternal fundeclarations. The guard test was also verified to fail when the C symbols are deliberately reverted.The merged manifest was inspected, not just the assemble result. All three relocated components appear in
app/build/intermediates/merged_manifests/debug/.../AndroidManifest.xmlwith their names,exportedflags, theme andforegroundServiceTypeintact. Manifest merging can succeed while producing a subtly wrong component.