Fix crash on IntelliJ import with dependsOn(RootProject(...)) (#146) - #147
Conversation
e3bbdcf to
fbf9da3
Compare
1764a49 to
98d85ff
Compare
98d85ff to
18da64f
Compare
|
|
||
| protected lazy val projectMap: Map[ProjectRef, ProjectDataType] = projectsData.iterator.map(x => x.thisProject -> x).toMap | ||
| protected lazy val revProjectMap: Seq[(ProjectRef, ProjectRef)] = projectsData.flatMap(x => buildDependencies.classpathRefs(x.thisProject).map(_ -> x.thisProject)) | ||
| protected lazy val revProjectMap: Seq[(ProjectRef, ProjectRef)] = projectsData.flatMap(x => buildDependencies.classpathRefs(x.thisProject).filter(projectMap.contains).map(_ -> x.thisProject)) |
There was a problem hiding this comment.
Could you leave a line comment here with explanation why filtering as per the projectMap is needed? For people not in the context this bit can be useful
There was a problem hiding this comment.
This ideally also needs a comment (Scaladoc?) to the projectMap
There was a problem hiding this comment.
The comment might also refer to the existing tests (see example here...)
There was a problem hiding this comment.
Done, all three:
- Inline comment on
revProjectMapexplaining the.filter(projectMap.contains)drops reverse edges to externaldependsOn(RootProject(...))projects — they
aren't inprojectMap, so keeping them would crash the laterprojectMap(ref)lookup incollectParents(the dependsOn(RootProject(...)) crashes on IntelliJ import when depending on a non-IntelliJ SBT project #146 bug). - Scaladoc on
projectMapitself: it holds only refs from the current build (projectsData); external refs are deliberately absent, which is why every lookup in
the trait is guarded withprojectMap.contains/projectMap.get. - Both now point to the renamed
SbtProjectStructureExtractorExternalRefsTestas the example.
| log.warn(s"skipping external project ref not part of the current build: $root") | ||
| queue | ||
| case Some(data) => | ||
| if (!queue.contains(root)) { |
There was a problem hiding this comment.
(minor) I would extract this branch to a method with a meaningful name
There was a problem hiding this comment.
Extracted into private def enqueueWithDependencies(data, root, queue). topoSortRefs is now just the projectMap.get(root) match (skip-external vs. recurse),
with the recursion body in the named method under its own Scaladoc.
|
|
||
| override def collectChildren(node: NodeType, data: ProjectDataType): Seq[NodeType] = { | ||
| val childRefs = buildDependencies.classpathRefs(node.ref) | ||
| val childRefs = buildDependencies.classpathRefs(node.ref).filter(projectCache.contains) |
There was a problem hiding this comment.
Also, a some small comment for the filtering would be nice to have, with reference to example
There was a problem hiding this comment.
Added a short comment on the .filter(projectCache.contains) in collectChildren, mirroring the revProjectMap rationale and pointing to #146 and
SbtProjectStructureExtractorExternalRefsTest.
| // for all nodes. This allows the plugin author to set packageLibraryMappings once | ||
| // on the root project and have it apply to external projects loaded via RootProject | ||
| // that don't have sbt-idea-plugin and therefore have empty libMapping. | ||
| val rootLibraryMappings = nodes |
There was a problem hiding this comment.
Worth extracting to a method with a Scaladoc
There was a problem hiding this comment.
Extracted into private def rootLibraryMappingsOf(nodes): Map[ModuleKey, Option[String]] with a Scaladoc explaining it returns the root (Standalone) project's
libraryMappings used as global defaults by processLibraries, and that it returns an empty map when there's no Standalone node.
| val data = keyFor.?.all(ScopeFilter(inAnyProject)).value.flatten.filterNot(_ == null) | ||
| val pluginData = keyFor.?.all(ScopeFilter(inAnyProject)).value.flatten.filterNot(_ == null) | ||
|
|
||
| // Collect packaging data for external projects loaded via RootProject/ProjectRef. |
There was a problem hiding this comment.
I understand how hard it is to work with SBT code and especially SBT tasks & all the macro magic inside.
Still, if you would manage to extract his to a separate subtask, this would simply the code
There was a problem hiding this comment.
Done. The cross-project collection (the *.all(ScopeFilter(inAnyProject)) reads + assembly) is now a separate sub-task, private def externalProjectData(keyFor, productsKey): Def.Initialize[Task[Seq[SbtPackageProjectData]]], with the explanatory Scaladoc moved onto it. packageMappingsImpl reduces to val externalData = externalProjectData(keyFor, productsKey).value. The online/offline productsKey split (products vs productDirectories) is preserved exactly, and the pure
assembly stays in PackagingKeysInit.buildExternalProjectData (unit-tested by PackagingKeysInitTest).
| import java.io.File | ||
| import java.net.URI | ||
|
|
||
| class SbtProjectStructureExtractorBaseTest extends AnyFunSuite with Matchers { |
There was a problem hiding this comment.
Why "*BaseTest". ATM it sounds a little misleading
With that name we usually expect some abstract class that is used as a base for other concrete tests.
There was a problem hiding this comment.
And could you please leave a Scaladoc with the description of what the test tests and how it's different from other tests / why other tests can't be reused
There was a problem hiding this comment.
Renamed the class and file to SbtProjectStructureExtractorExternalRefsTest — it now names what's verified (the trait's handling of external ProjectRefs) instead
of implying an abstract base for other tests.
Also added a class Scaladoc describing what it covers (regression coverage for #146) and why the existing integration-style structure tests can't: those load a real
sbt build where every project shares one build, so no ref is ever "external". Here we drive the trait with a synthetic graph — the only way to produce a ref that's
on the classpath but missing from projectMap.
| } | ||
|
|
||
| /** | ||
| * Test-friendly extractor that uses a simple adjacency map instead of requiring |
There was a problem hiding this comment.
This looks like quite a big class with quite some code duplication.
As per the comment the only reason it exists is because BuildDependencies has private constructor.
But from what I see the factory method sbt.internal.BuildDependencies.apply is public and you can use it. Why not using it?
There was a problem hiding this comment.
Even if that still doesn't work, I would rather adopt org.jetbrains.sbtidea.structure.sbtImpl.SbtProjectStructureExtractorBase to be more test friendly and decouple it from BuildDependencies by introducing an interface exposing what we actually need from BuildDependencies
(AFAIU it's just sbt.internal.BuildDependencies#classpathRefs)
There was a problem hiding this comment.
Good call on the factory — it works, so TestExtractor no longer overrides revProjectMap / topoSortRefs / collectChildren. The test now builds a real
BuildDependencies via BuildDependencies.apply(classpath, aggregate) (small mkBuildDependencies helper that wraps each dep as ResolvedClasspathDependency(_, None)), and TestExtractor implements only the genuinely abstract members (buildStub, updateNode, collectLibraries). The duplication is gone and the
production traversal logic runs verbatim.
Since the factory worked, I didn't introduce the classpathRefs-only interface to decouple the extractor from BuildDependencies — that avoided touching production
code purely for test ergonomics. Happy to add it if you'd prefer the decoupling on principle; let me know.
| } | ||
| } | ||
|
|
||
| private def node( |
There was a problem hiding this comment.
Were these test utilities written based on example in LinearMappingsBuilderMergeWarningsTest?
I now see duplication and it becomes messy.
I would expect some cleaning up an dunification
There was a problem hiding this comment.
Yes — those helpers were copied from LinearMappingsBuilderMergeWarningsTest. Unified them into a single shared PackagingTestNodes trait (one parameterized
node(...) / packagingOptions(...) plus TestNode / TestLibrary / mkKey, all optional fields defaulting to empty). Both ExternalProjectPackagingTest and
LinearMappingsBuilderMergeWarningsTest now mix it in and dropped their local copies.
…ins#146) packageMappingsOffline now reads (Compile / productDirectories) instead of (Compile / products) for the cross-project collection of external RootProjects. Without this, IntelliJ sync (which calls packageMappingsOffline via createIDEAArtifactXml on every onLoad) force-compiles every project in the build, and a compile error in any external RootProject breaks sync entirely with "extracting project structure from sbt: failed". Online packageMappings continues to use (Compile / products) since real packaging needs compiled class files on disk. This mirrors the existing online/offline split in dumpDependencyStructure(Offline). The external-project data assembly is extracted to PackagingKeysInit.buildExternalProjectData so the plumbing can be unit-tested without an SBT environment; PackagingKeysInitTest covers it.
18da64f to
acbcd38
Compare
|
Thanks for the thorough review, Dmitrii Naumenko (@unkarjedy) — all comments addressed. A couple of heads-ups first:
What changed, by area:
|
|
teeckoo (@teeckoo) Thanks! |
This is an appended fix for the external RootProject packaging fix merged in PR #147 for issue #146. It covers newly found edge cases where root packageLibraryMappings target an external-only non-Scala library or are read from the wrong Standalone node when another standalone node appears before the actual root plugin project. Root mappings now come from the actual root plugin project and are validated against the full packaging graph, while node-specific mappings remain validated against node-local libraries. Co-authored-by: Codex <codex@openai.com>
Fix
dependsOn(RootProject(...))crash and packaging (#146)Fixes two crashes during IntelliJ import when an IJ plugin project uses
dependsOn(RootProject(...))to depend on a normal SBT project, and ensures external project class files and libraries are included in the plugin artifact.Crash fixes
SbtProjectStructureExtractorBase.scala—topoSortRefscrashed withNoSuchElementExceptionwhenbuildDependencies.classpathRefsreturned an externalProjectRefnot inprojectMap. Changed toprojectMap.get(root)with pattern match; filter external refs intopoSortRefs,revProjectMap, andcollectChildren.Init.scala—createIDEARunConfigurationaggregatedintellijExtraRuntimePluginsInTestsviaScopeFilter(inDependencies(ThisProject)), which reached external projects without sbt-idea-plugin. Added defaults forintellijPluginsandintellijExtraRuntimePluginsInTestsinglobalSettings.External project packaging
PackagingKeysInit.scala— External projects don't have sbt-idea-plugin in their builds, sodumpDependencyStructurewas undefined for them and they were excluded from the packaging graph.packageMappingsImplnow collects packaging data for all projects using standard SBT keys (products,managedClasspath,libraryDependencies,updateFull). External projects default toMergeIntoParent()with emptylibMapping, so their class files and libraries are automatically merged into the plugin artifact.Root project
packageLibraryMappingsas global filterLinearMappingsBuilder.scala—processLibrariesnow merges the root (Standalone) project'slibraryMappingsas global defaults with each node's own mappings. Node-specific mappings override root mappings. This lets the plugin author setpackageLibraryMappingsonce on the root project and have it apply to all nodes — including external projects that have nopackageLibraryMappingsof their own.README.md— Added "Source-Level Debugging withpackageArtifactDynamic" section documenting the three packaging tasks, external project support, and globalpackageLibraryMappingsbehavior.Tests
SbtProjectStructureExtractorBaseTest.scala— verify structure extraction filters external refs without crashingExternalProjectPackagingTest.scala— verify external project classes and libraries are included in packaging, root exclusions filter child libraries, node overrides root, and no-op when no external deps existResult
No
packageFileMappingsor manuallibraryDependenciespull needed.packageArtifactDynamicenables source-level debugging across module boundaries.packageLibraryMappingson the root project controls library exclusions globally — including external projects:Root mappings act as defaults; node-specific mappings override:
libMappinglibMappingroot ++ node)scala-.* -> Nonescala-.* -> Nonescala-.* -> None(subproject)scala-.* -> Nonescala-.* -> Nonescala-reflect -> Some("lib/")scala-.* -> Nonescala-reflect -> Some("lib/")For projects that don't use
dependsOn(RootProject(...)), all subprojects already have sbt-idea-plugin enabled and are covered bydumpDependencyStructure. The new external project collection produces an empty list, so the packaging behavior is identical to before.Minimal reproducible example
https://github.com/collaboncode/minimal-ij-plugin-setup