Repository navigation
fix(discover): keep JVM packages named like build dirs (part of #2661) - #2724
Conversation
|
Thanks for opening this — it has been seen, and it is queued. This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence. Current review status: working through a backlog. What that means for this PR, concretely:
Things that will genuinely speed it up whenever review does happen:
If this fixes a bug, a reproduction we can run is worth more than a description of the symptom. Thanks for contributing, and sorry in advance for the wait. |
DeusData
left a comment
There was a problem hiding this comment.
Thank you, @tmonestudio, this is a really nice fix, and thank you for the careful description. You found the real cause of #2661 (the basename match against ALWAYS_SKIP_DIRS), you kept the exception tight (only strictly below src/<set>/<lang>/, never the language root or its siblings, never the safety core), and you applied the same predicate in discovery and in the inventory filter so the two agree. The tests cover both the predicate and a real discovery walk, including the target/classes and .git cases that must stay skipped.
We built your branch and tried it on a real repository. In elasticsearch, the core org.elasticsearch.env package (22 files: Environment, NodeEnvironment, ShardLock and friends) is never indexed on main. With your change it is, and 1,465 calls that had no target before now bind into it. clang-format and cppcheck 2.20 (the version CI uses) are clean on your changed source files. The CI run on this head was cancelled by a hang in our checkout step, not by anything in your change; it runs again on your next push.
A few things before we can merge:
-
Please move the GLSL commit (
feat(language): map GLSL compute, geometry and tessellation stages) into its own PR. It is a good change, but it is a separate feature from #2661, and we keep one claim per PR so each can be reviewed, reverted and bisected on its own. It is already its own commit, so this should be quick, and we are happy to review the GLSL PR right away. One heads-up for it: we indexed a small compute, geometry and tessellation shader with your mapping, and the files now index, but our vendored GLSL grammar reads the qualifier-onlylayout(...) in;/layout(...) out;lines as parse errors, so a definition right after one of them can be missed. That is a limitation on our side, not something your PR needs to fix. -
Please change
Fixes #2661toPart of #2661. The issue's own reproducer (from #2382) keeps its class atsrc/com/example/target/TargetDTO.java, a plain layout without themain/javalevel, and with this change that package is still skipped (we checked with your branch). Keeping the issue open lets that layout be handled separately; if you would like to take it on in a follow-up PR, you are very welcome to. -
In
moderateandfastmode,cbm_should_skip_diralso applies the fast-mode skip list (generated,gen,tools,integration,examples,docs, ...), so your exception keeps those package names too when they sit below a JVM source root. We think that is the right behavior, since they are packages there as well (in elasticsearch that is about 470 more files in moderate mode, for exampleorg.elasticsearch.xpack.core.ml.integration), but it is not mentioned yet. Could you add a sentence about it to the description, and pin it with aCBM_MODE_MODERATEcase indiscover_keeps_jvm_packages_named_like_build_dirs_issue2661, for example asrc/main/java/com/acme/integration/I.javathat is kept while a top-levelintegration/stays skipped? Users who want the fast tiers to stay lean there can already exclude such a package with a.cbmignorepattern (and a!dir/negation re-includes a built-in skip), so no new setting is needed.
Thanks again. This closes a real hole in how Java and Kotlin projects are indexed.
…ta#2661) Below src/<set>/{java,kotlin,scala,groovy}/ every directory is a package, so a built-in skip name there (target, tmp, vendor, deploy, env, dist...) is a package name, not build output. Discovery and the inventory filter now keep such directories; the root itself, its siblings and the safety core (.git, worktrees) stay skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Leonardo trindade miranda <tmonestudio@gmail.com>
Below a JVM source root, a package named like a fast-mode skip dir (integration/) is kept in moderate mode, while a top-level integration/ stays skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: mbrl <mbrl@local>
08b1267 to
d57846c
Compare
|
Thanks for the review. Done: (1) the GLSL commit is now its own PR, #2761, with the grammar limitation noted; (2) the description now says |
…eusData#2661) The Java and Kotlin doc-link reason tests modelled "a package in a directory discovery skips" with src/main/java/a/env/ and src/main/kotlin/k/env/. Discovery now keeps packages below src/<set>/<lang>/, so those files are indexed and the two not_indexed assertions failed. Move both fixtures to a plain src/<pkg>/ layout, which discovery still skips by name (the open part of DeusData#2661), and reword the comments. Assertions unchanged. Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
|
Thank you, @tmonestudio, for working through all three points so carefully. The GLSL stage mapping now stands on its own and is merged as #2761, the description says exactly what the change does (Part of #2661, plus the moderate/fast-mode behavior), and the new CBM_MODE_MODERATE case in discover_keeps_jvm_packages_named_like_build_dirs_issue2661 pins that behavior. The production change is the same one we measured on elasticsearch, so nothing more is needed there. The remaining red CI was not caused by a problem in your fix but by two of our older tests, so we pushed one small commit on top of your branch (f05ecce):
Since we pushed to your branch, please pull before pushing again. Thanks again for closing this hole in how Java and Kotlin projects are indexed. |
|
Merged as 4edc6b7 - thank you, @tmonestudio! Java and Kotlin packages whose directory names look like build folders are no longer skipped, which closes a real hole in how JVM projects are indexed (part of #2661). Thank you for working through every review point so carefully. |
What does this PR do?
Part of #2661.
JVM packages named like build output (#2661).
ALWAYS_SKIP_DIRSmatches a directory by basename anywhere in the tree, sosrc/main/java/com/acme/target/Foo.javawas never indexed and calls into it bound to whatever else shared the name. Belowsrc/<set>/{java,kotlin,scala,groovy}/every directory is a package, so a built-in skip name there (target,tmp,temp,vendor,deploy,env,dist,obj,coverage) is now kept.target/classes,src/main/targetandsrc/main/javaitself keep the existing behavior..git, worktrees) is never un-skipped.cbm_discover_path_in_jvm_package) is applied in both places that consult the built-in list:directory_verdictin discovery andcif_directory_reasonin the inventory filter.moderateandfastmode the exception also covers the fast-mode skip list (generated,gen,tools,integration,examples,docs, ...): below a JVM source root those names are packages too, so they are kept (e.g.src/main/java/com/acme/integration/I.java), while the same directory at the top level (integration/) stays skipped. This is pinned by aCBM_MODE_MODERATEcase indiscover_keeps_jvm_packages_named_like_build_dirs_issue2661. Users who want the fast tiers to stay lean there can exclude such a package with a.cbmignorepattern.The plain layout from the #2382 reproducer (
src/com/example/target/TargetDTO.java, without themain/javalevel) is still skipped, so #2661 stays open for that case.The GLSL stage mapping that was in this PR moved to its own PR, as requested.
Note on #2382
While reproducing #2382 I found that on current
mainthe typed-parameter case resolves correctly on the parallel pipeline path and fails only on the sequential one (small repos). #2667 builds the Java cross registry on the sequential path too; with it merged, the #2382 reproducer resolves correctly on both paths. So this PR does not touch call resolution. Details in a comment on #2382.Checklist
git commit -s)maindiscover,language,inventory_filter,gitignore,gitignore_checked,workspacelocally (445/0/4 skipped); the new moderate-mode case has not been run locally, it relies on CIjvm_package_path_predicate_issue2661,discover_keeps_jvm_packages_named_like_build_dirs_issue2661End to end with the built binary: in a Maven layout with
com/acme/target/Foo.java,Foo.javais now indexed andf.ping()binds tocom.acme.target.Foo.ping.target/classes/*.javaat the module root stays skipped.🤖 Generated with Claude Code