#2178: Make Release commandlet build-tool independent - #2302
Conversation
Coverage Report for CI Build 31779846274Warning No base build found for commit Coverage: 72.894%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
laert-ll
changed the title
…dent-of-specific-build-commandlet
maybeec
left a comment
maybeec
left a comment
There was a problem hiding this comment.
Thanks for picking this up 👍 — and thanks especially for not stopping at the literal ask. Turning the detection loop from last match wins into first match wins (BuildCommandlet#findBuildCommandlet) quietly fixes two real bugs that were sitting in BuildCommandlet:
- In a yarn project (
package.json+yarn.lock) the old loop had nobreak, so afterYarnmatched it kept going andNpm— last inBUILD_TOOLS— always took over.Yarn.findBuildDescriptorwas effectively unreachable. - Worse, in a polyglot repo (
pom.xmlandpackage.json) the old code rannpmbut resolved the defaults fromMVN_BUILD_OPTS, i.e. it executednpm clean install. Hoisting theargs.isEmpty()block out of the loop fixes that too.
Also good: the parent == null guard in isTopLevelProject — the old projectPath.getParent().resolve("pom.xml") would have thrown a raw NPE when releasing from a filesystem root.
No blockers. The instanceof BuildTool gate is the right shape, CliException is the correct exception type here (expected, user-facing abort), no public signature was broken, and the tests stay inside target. A few things I'd like to see before merge:
Should-fix
ReleaseCommandlet:45-48— the error message collapses "no build descriptor at all" and "build tool found but it cannot release" into one text. A Gradle user gets told nothing useful.BuildCommandlet:73— the new detection is a package-privatestaticthatReleaseCommandletreaches across into, and it takes anIdeContextparameter although both callers already havethis.context. The issue itself points at the better home: "Ideally we should ask the commandlet manager".BuildCommandlet:75—findBuildCommandletNPEs on anullpath;BuildCommandletguards before calling,ReleaseCommandletdoes not.BuildCommandletTest:142— the yarn-over-npm fix is the user-visible part of this PR but is only covered at helper level; no test asserts thatide buildin the yarn workspace actually runsyarn, and nothing covers the mvn-vs-npm precedence flip.
Minor
ReleaseCommandlet:106— first condition is always true at the only call site.ReleaseCommandlet:102— parameter namedbuildToolbut typedLocalToolCommandlet, whiledoRunhas a different variable also calledbuildTool.BuildCommandlet:55— error message still hardcodes the descriptor list that this PR is de-hardcoding.CHANGELOG.adoc:9— theide buildprecedence fix is user-facing and not mentioned.
PR hygiene
- The branch is BEHIND
mainand needs an update before merge. - The checklist ticks "PR and issue(s) have suitable labels", but this PR carries no labels and no milestone. Per DoD.adoc both should be set before close (the issue itself is labelled fine).
Manual verification
Green CI is not sufficient for this one. ide release drives git commit/git tag/git push and a real deploy build — none of which the mock GitContextImplMock exercises end-to-end. Please do run your own testing instructions (2) and (3) on a real Maven project before merge, and note the result in the PR.
Scope against #2178: the primary ask ("ReleaseCommandlet should ask the available build commandlets if they apply") is met. The Additional context part — discovering build commandlets dynamically via the commandlet manager and expressing the yarn/npm priority without a hardcoded list — is not addressed; BUILD_TOOLS is untouched. That is explicitly framed as "ideally" in the issue, so I am not blocking on it, but please say in the PR whether you consider it out of scope so it can be tracked as a follow-up rather than silently dropped.
| LocalToolCommandlet commandlet = BuildCommandlet.findBuildCommandlet(this.context, projectPath); | ||
| if (!(commandlet instanceof BuildTool buildTool)) { | ||
| throw new CliException("Could not find a supported build tool to release the project in " + projectPath + "."); | ||
| } |
There was a problem hiding this comment.
Should-fix — this branch folds two genuinely different failures into one message:
commandlet == null→ there is no build descriptor here at all, so there is nothing to release;commandletis non-nullbut not aBuildTool→ we did detect the project's build tool (e.g.Gradleviabuild.gradle), it simply has no release support yet.
A user standing in a Gradle project is told "Could not find a supported build tool", which reads as "your project is broken" when the truth is "gradle release is not implemented". Naming the detected tool turns a dead end into an actionable message — and it is the difference between the user filing a bug and the user opening a feature request.
| LocalToolCommandlet commandlet = BuildCommandlet.findBuildCommandlet(this.context, projectPath); | |
| if (!(commandlet instanceof BuildTool buildTool)) { | |
| throw new CliException("Could not find a supported build tool to release the project in " + projectPath + "."); | |
| } | |
| LocalToolCommandlet commandlet = BuildCommandlet.findBuildCommandlet(this.context, projectPath); | |
| if (commandlet == null) { | |
| throw new CliException("Could not find a build descriptor in " + projectPath + " - there is nothing to release here."); | |
| } | |
| if (!(commandlet instanceof BuildTool buildTool)) { | |
| throw new CliException("The build tool " + commandlet.getName() + " detected in " + projectPath + " does not support releasing."); | |
| } |
Note this also makes ReleaseCommandletTest#testReleaseWithoutBuildDescriptor (which currently only asserts CliException.class) distinguishable from your new testReleaseWithUnsupportedBuildToolThrowsException — worth tightening that older test with a hasMessageContaining too, the way you already did for the new one.
| * @param buildPath the {@link Path} to the directory to build. | ||
| * @return the applicable build {@link LocalToolCommandlet} or {@code null} if no build descriptor was found. | ||
| */ | ||
| static LocalToolCommandlet findBuildCommandlet(IdeContext context, Path buildPath) { |
There was a problem hiding this comment.
Should-fix — placement. Two things about this signature:
- It takes
IdeContext contextas a parameter although both call sites already holdthis.context. Passing the context around when the owning object already has it injected is the pattern we avoid across the codebase. - It makes
ReleaseCommandletdepend on a package-privatestaticinside a sibling commandlet. Commandlets are peers dispatched by the same manager, so one reaching into another's internals is exactly the separation-of-concerns smell Make ReleaseCommandlet independent of specific build commandlet #2178 set out to remove.
#2178 names the better home directly: "Ideally we should ask the commandlet manager for all commandlets that are build commandlets and then ask them for the build descriptor." CommandletManager already exposes getCommandlets(), so a LocalToolCommandlet findBuildCommandlet(Path) there would keep BUILD_TOOLS and the priority ordering as an implementation detail of the manager, and both callers collapse to:
LocalToolCommandlet commandlet = this.context.getCommandletManager().findBuildCommandlet(projectPath);That would also give Intellij#importRepository (Intellij.java:158-172) — which today runs a third copy of this "iterate build tools, first descriptor wins" loop over its own BUILD_TOOL_TO_IJ_TEMPLATE map — somewhere to converge later.
If you would rather not touch CommandletManager in this PR, the minimum I would ask for is dropping the context parameter and making this an instance method, with ReleaseCommandlet obtaining the commandlet the normal way (getCommandlet(BuildCommandlet.class)). Not blocking on the exact home — but the reach-across plus redundant parameter should go.
| */ | ||
| static LocalToolCommandlet findBuildCommandlet(IdeContext context, Path buildPath) { | ||
|
|
||
| for (Class<? extends LocalToolCommandlet> toolClass : BUILD_TOOLS) { |
There was a problem hiding this comment.
Should-fix — null contract. Mvn.findBuildDescriptor does directory.resolve(POM_XML), so a null buildPath produces a raw NullPointerException from inside the loop rather than a clean CLI error.
BuildCommandlet#doRun guards this at line 49 before calling, but ReleaseCommandlet#doRun now calls findBuildCommandlet(this.context, this.context.getCwd()) as its very first use of the cwd with no such guard. Previously git.hasUntrackedFiles(projectPath) was the first consumer, so this is not a regression you introduced — but extracting the shared helper is precisely the moment to fix the asymmetry, rather than leaving each caller to remember.
Moving the check into the helper covers both callers and keeps doRun shorter:
static LocalToolCommandlet findBuildCommandlet(IdeContext context, Path buildPath) {
if (buildPath == null) {
throw new CliException("Missing current working directory!");
}
for (Class<? extends LocalToolCommandlet> toolClass : BUILD_TOOLS) {The existing testBuildWithNoCwd keeps passing, and ide release with no cwd stops NPE-ing. See coding-conventions.adoc § Avoid catching NPE — the rule there is to check for null explicitly instead of letting an NPE happen, and constructing an exception with a full stack collect is the expensive path we should not take for a condition a single if covers.
| assertThat(BuildCommandlet.findBuildCommandlet(context, workspace.resolve("gradle"))).isInstanceOf(Gradle.class); | ||
| assertThat(BuildCommandlet.findBuildCommandlet(context, workspace.resolve("npm"))).isInstanceOf(Npm.class); | ||
| // both npm and yarn match package.json, but yarn.lock is present so yarn must take precedence over npm | ||
| assertThat(BuildCommandlet.findBuildCommandlet(context, workspace.resolve("yarn"))).isInstanceOf(Yarn.class); |
There was a problem hiding this comment.
Should-fix — coverage gap on the behaviour that actually changed for users.
This asserts the helper returns Yarn, which is good, but the previously broken behaviour was one level up: because the old loop in doRun had no break, a yarn project was really built by npm. Nothing here asserts that ide build in workspaces/main/yarn now logs yarn run build. The existing testNpmBuildWithProvidedArguments shows the shape (IdeLogEntry.ofInfo("npm start test")); a yarn equivalent would pin the fix so a future reordering of BUILD_TOOLS cannot silently undo it. I appreciate that needs _ide/urls/yarn + repository fixtures in the build test project, so it is more work than one line — but it is the assertion that proves the bug is dead.
The second uncovered flip is quieter and worth at least a helper-level assertion here, since it costs nothing: a directory containing both pom.xml and package.json. Old code ran npm with MVN_BUILD_OPTS (npm clean install); new code correctly picks Mvn. Adding a mvn-and-npm fixture folder and one more line:
// a polyglot project must be built by the highest-priority tool, not the last one that matches
assertThat(BuildCommandlet.findBuildCommandlet(context, workspace.resolve("mvn-and-npm"))).isInstanceOf(Mvn.class);would document the priority contract that BUILD_TOOLS now carries.
| && !Files.exists(projectPath.getParent().resolve("pom.xml")); | ||
| // top-level if a build descriptor is present here but not in the parent directory | ||
| Path parent = projectPath.getParent(); | ||
| return (buildTool.findBuildDescriptor(projectPath) != null) |
There was a problem hiding this comment.
Minor — the first operand is dead. At the only call site (line 57) commandlet came out of findBuildCommandlet(this.context, projectPath), which returns a commandlet only when findBuildDescriptor(projectPath) != null. So this re-runs a filesystem Files.exists check whose answer is already known to be true.
The method reduces to the question it is actually asking:
| return (buildTool.findBuildDescriptor(projectPath) != null) | |
| // top-level if the build descriptor found here is not also present in the parent directory | |
| Path parent = projectPath.getParent(); | |
| return (parent == null) || (buildCommandlet.findBuildDescriptor(parent) == null); |
(uses the renamed parameter from the comment above). Non-blocking — the current code is correct, just doing redundant I/O.
| } | ||
|
|
||
| private boolean isTopLevelProject(Path projectPath) { | ||
| private boolean isTopLevelProject(LocalToolCommandlet buildTool, Path projectPath) { |
There was a problem hiding this comment.
Minor — naming. This parameter is called buildTool but is typed LocalToolCommandlet, while doRun binds a different variable also named buildTool — of type BuildTool (line 46) — to the very same instance. Two names for one object, and the shared name is the one that does not match this type. Reading isTopLevelProject in isolation, buildTool suggests you are holding the BuildTool interface, which is the type that cannot answer findBuildDescriptor.
| private boolean isTopLevelProject(LocalToolCommandlet buildTool, Path projectPath) { | |
| private boolean isTopLevelProject(LocalToolCommandlet buildCommandlet, Path projectPath) { |
(and the two usages in the body). coding-conventions.adoc § Naming — "always use short but speaking names"; here the name actively points at the wrong abstraction.
| } | ||
| LocalToolCommandlet commandlet = findBuildCommandlet(this.context, buildPath); | ||
| if (commandlet == null) { | ||
| throw new CliException("Could not find build descriptor - no pom.xml, build.gradle, or package.json found!"); |
There was a problem hiding this comment.
Minor — not introduced by you, but this PR is the one that de-hardcodes build-tool knowledge, so it stands out now. This message enumerates the descriptors by hand and has already drifted: Gradle.findBuildDescriptor also accepts build.gradle.kts, and it says nothing about yarn.lock. Every future entry in BUILD_TOOLS has to remember to edit this string, which is exactly the coupling the new helper removes elsewhere.
Since findBuildCommandlet already walks the tools, the message could be derived instead of restated — or, cheapest, drop the enumeration:
| throw new CliException("Could not find build descriptor - no pom.xml, build.gradle, or package.json found!"); | |
| throw new CliException("Could not find a build descriptor in " + buildPath + " - no supported build tool detected."); |
Entirely optional and safe to leave for a follow-up.
|
|
||
| Release with new features and bugfixes: | ||
|
|
||
| * https://github.com/devonfw/IDEasy/issues/2178[#2178]: Make ReleaseCommandlet independent of specific build commandlet |
There was a problem hiding this comment.
Minor — the entry only advertises the ReleaseCommandlet refactoring, but the most visible effect of this PR for existing users is on ide build: yarn projects were being built with npm, and polyglot repos were being built with the wrong tool and the wrong default options. That is a user-facing bugfix and per DoD.adoc belongs in the changelog, otherwise nobody upgrading will connect a changed ide build behaviour to this issue.
| * https://github.com/devonfw/IDEasy/issues/2178[#2178]: Make ReleaseCommandlet independent of specific build commandlet | |
| * https://github.com/devonfw/IDEasy/issues/2178[#2178]: Make ReleaseCommandlet independent of specific build commandlet and fix `ide build` using npm instead of yarn |
…dent-of-specific-build-commandlet
4 participants
This PR fixes #2178
Implemented changes:
Testing instructions
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat). If no issue ID exists, title only.In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed, you have updated our LICENSE.asciidoc