Skip to content

fix: missing source and jar deps now handled correctly #2252

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: missing source and jar deps now handled correctly #2252

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions itests/brokendeps_jar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
//DEPS missing.jar

class brokendeps {
public static void main(String... args) throws Exception {
}
}
6 changes: 6 additions & 0 deletions itests/brokendeps_source.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
//DEPS missing.java

class brokendeps_source {
public static void main(String... args) throws Exception {
}
}
17 changes: 16 additions & 1 deletion src/main/java/dev/jbang/cli/Info.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,7 @@ static class ScriptInfo {
String gav;
String module;
Map<String, List<ProjectFile>> docs;
List<ProjectFile> subProjects;

public ScriptInfo(Project prj, Path buildDir, boolean assureJdkInstalled) {
originalResource = prj.getResourceRef().getOriginalResource();
Expand Down Expand Up @@ -181,10 +182,24 @@ private void init(Project prj) {
if (!opts.isEmpty()) {
runtimeOptions = opts;
}
List<Project> subps = prj.getSubProjects();
if (!subps.isEmpty()) {
subProjects = subps.stream()
.map(Project::getResourceRef)
.map(ProjectFile::new)
.collect(Collectors.toList());
if (dependencies == null) {
dependencies = new ArrayList<>();
}
dependencies.addAll(
subps.stream().map(p -> p.getResourceRef().getOriginalResource()).collect(Collectors.toList()));
}
}

private void init(SourceSet ss) {
List<String> deps = ss.getDependencies();
List<String> deps = new ArrayList<>();
deps.addAll(ss.getDependencies());
deps.addAll(ss.getClassPaths());
if (!deps.isEmpty()) {
dependencies = deps;
}
Expand Down
13 changes: 12 additions & 1 deletion src/main/java/dev/jbang/dependencies/DependencyResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
package dev.jbang.dependencies;

import java.io.File;
import java.nio.file.Path;
import java.util.*;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import org.jspecify.annotations.NonNull;

import dev.jbang.resources.ResourceNotFoundException;
import dev.jbang.util.Util;

public class DependencyResolver {
Expand Down Expand Up @@ -73,10 +77,17 @@ public ModularClassPath resolve() {
// WARN need File here because it's more lenient about paths than Path!
Stream<ArtifactInfo> cpas = classPaths
.stream()
.map(p -> new ArtifactInfo(null, new File(p).toPath()));
.map(p -> new ArtifactInfo(null, assertExists(new File(p).toPath())));
List<ArtifactInfo> arts = Stream.concat(mcp.getArtifacts().stream(), cpas)
.collect(Collectors.toList());
return new ModularClassPath(arts);
}
}

private @NonNull Path assertExists(@NonNull Path p) {
if (!p.toFile().exists()) {
throw new ResourceNotFoundException(p.toString(), "Jar dependency not found: " + p);
}
return p;
}
}
29 changes: 29 additions & 0 deletions src/main/java/dev/jbang/dependencies/DependencyUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.function.Predicate;
import java.util.regex.Matcher;
import java.util.regex.Pattern;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -197,4 +198,32 @@ public static MavenRepo toMavenRepo(String repoReference) {
return new MavenRepo(repoid, reporef);
}
}

public static Predicate<String> gavDepFilter() {
return DependencyUtil::isGav;
}

private static boolean isGav(String ref) {
return looksLikeAPossibleGav(ref) || !JitPackUtil.ensureGAV(ref).equals(ref);
}

public static List<String> filterGavDeps(List<String> deps) {
return deps.stream().filter(gavDepFilter()).collect(Collectors.toList());
}

public static Predicate<String> jarDepFilter() {
return d -> d.endsWith(".jar");
}

public static List<String> filterJarDeps(List<String> deps) {
return deps.stream().filter(jarDepFilter()).collect(Collectors.toList());
}

public static Predicate<String> sourceDepFilter() {
return gavDepFilter().negate().and(jarDepFilter().negate());
}

public static List<String> filterSourceDeps(List<String> deps) {
return deps.stream().filter(sourceDepFilter()).collect(Collectors.toList());
}
}
36 changes: 21 additions & 15 deletions src/main/java/dev/jbang/source/ProjectBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,8 @@ private Project createJbangProject(ResourceRef resourceRef) {

SourceSet ss = prj.getMainSourceSet();
ss.addResources(allToFileRef(directives.files(), resourceRef, sibRes1));
ss.addDependencies(directives.binaryDependencies());
ss.addDependencies(DependencyUtil.filterGavDeps(directives.dependencies()));
Copy link
Collaborator

@maxandersen maxandersen Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so we add gavs first, jars second?

if thats the case shouldn't jars rather be included via //classpath rather than //deps?

Copy link
Contributor Author

@quintesse quintesse Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

//classpath doesn't exist and we've supported jars inside //deps for a long time already. So not sure if you want to change that at this point?

Copy link
Contributor Author

@quintesse quintesse Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also support //deps some.java, would that also need to become //classpath some.java?
Personally I'd rather have a single //deps and let us detect what to with each item based on its type (gav, jar, source) rather than different directives for each. (In the end it's all about jars on the classpath)

Copy link
Collaborator

@maxandersen maxandersen Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what i'm concerned about is that ordering suddenly is not clean/simple.

And I do think we should have something mathching --classpath as it has different semantics.

I think it was yourself who told me at some point .jar's shouldn't be in //deps because they dont have transitive deps..so they behave differently.

so my question is just how to describe the ordering that gets applied?

is it deps first, then .java, then .jar, then what is on --classpath? ..is that forever-ever ? :)

Copy link
Contributor Author

@quintesse quintesse Oct 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what i'm concerned about is that ordering suddenly is not clean/simple.

I'm not sure why you say it's now not "clean and simple"?
Ordering has always been "by type", so artifacts first then any extra classpath elements.

And that hasn't changed much in this PR.
Before we had this for binary dependencies (which only returned GAVs):

ss.addDependencies(src.collectBinaryDependencies());

now we have the exact same result with:

ss.addDependencies(DependencyUtil.filterGavDeps(src.collectDependencies()));

And we had this for source dependencies, which by accident included JARs:

for (String srcDep : src.collectSourceDependencies()) {
	ResourceRef subRef = sibRes1.resolve(srcDep, true);
	prj.addSubProject(new ProjectBuilder(buildRefs).build(subRef));
}

and this is what we do with sub projects:

public ModularClassPath resolveClassPath() {
   ...
		for (Project prj : project.getSubProjects()) {
			prj.updateDependencyResolver(resolver);
			resolver.addClassPath(forSubProject(prj).getJarFile().toAbsolutePath().toString());
		}
   ...
}

As you can see any sub projects get added to the "classpath" elements, which mean they appear after the GAV artifacts.

The only difference with this PR is that we no longer treat JARs as source dependencies (= sub projects) anymore. Which you see in the new code, where instead of adding them to the "classpath" elements in a roundabout way we add them directly and explicitly:

ss.addClassPaths(DependencyUtil.filterJarDeps(src.collectDependencies()));

The only difference in ordering this PR creates is that it's now clearly and simply this ordering:

  1. Artifacts
  2. JARs
  3. Sub projects

instead of:

  1. Artifacts
  2. Some mix of Sub projects and JARs

So it's an improvement :-)

Copy link
Contributor Author

@quintesse quintesse Oct 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but why are you mentioning it here?

Edit: if it's to show that we have a special flag for setting elements that get added to the classpath, yes, that's true, but --class-path isn't really treated the same as --deps. It's a much simpler thing that just literally copies what you type there to the class path, no processing is done. So you can't reference jars that are relative to the script, you also can't reference remote elements or anything that is normally done via ResourceRefs. But it can reference class files in a folder, which is something --deps can't do.

Edit2: also, IIRC, the --class-path flag was purposely not added as a directive because it depended completely on your local situation which just didn't make sense as a directive.

Edit3: looked it up in the original PR:

This allows for adding additional classpath entries. Either by referring
to directories with classes or JAR files (--cp) or by referring to
Maven GAVs or remote JAR files (--deps)

So --cp was meant for local files and --deps for remote ones. Thing is that enforcing remote on --deps is weird, it would mean explicitly filtering out things it can do but shouldn't, which means --deps can also access local files. On the other hand allowing --cp to access remote files would mean adding a bunch of ResourceRef handling (and making sure that folders are treated as a special case where we don't use ResourceRefs).

So honestly --cp has become somewhat useless if the only thing it can do that --deps can't is folders with class files. Seems to me that we could perhaps try to make --deps support folders and then make --cp an alias of --deps (and perhaps deprecate it).

And yes, I might have said at some point that JARs aren't like other dependencies because they don't have their own dependencies, but I'm not sure it's relevant from a UX standpoint. We probably just want something that's easy to use.

Copy link
Collaborator

@maxandersen maxandersen Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes so unless i'm missing something, the .jar support in //DEPS has no difference in behavior on how --class-path works has it?

like, I'm not following what you mean you can't reference jars relative to the script - on the command line it would be relative to current working directory; not the script. That (relative to script) would be something a //CLASSPATH directive would be able to do.

Since the PR's code is already "reordering" deps ....so you get:

//DEPS ../lib/mylocal.jar
//DEPS otherproject.java
//DEPS g:a:v

but the actual order on classpath is:

g:a:v, ../lib/mylocal.jar, otherproject.java[.jars]

if that is the case, I'm saying I think that breaks (and have been broken ever since we did subprojects) the general rule of a directive takes precedent in the order found. i.e.

//JAVA 21
//JAVA 25

java 21 is supposed to win.

//DEPS g:a:1.2
//DEPS g:a:2.4

g:a:1.2 wins over g:a:2.4...

thus I'm suggestiong we stop overloading //DEPS with something that makes it so the sequence is affected by what is on the side of the directive.

i.e. (names just a suggestion, not stuck on them specifically):

//JARS ../lib/mylocal.jar
//SUBPROJECTS otherproject.java
//DEPS g:a:v

would be way easier to explain stating the order is DEPS, JARDEPS then SUBPROJECTS
and its much more explicit and no overloading of deps.

Copy link
Contributor Author

@quintesse quintesse Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I tried to explain why I think that's not the way to go, but sure.

Copy link
Collaborator

@maxandersen maxandersen Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets not close so hasty - i'm not sure on this :)

can you reiterate/try again explay why you think its better we make //DEPS handle all these cases? I read it as you mainly did it that way because //DEPS kinda already was starting to do it but maybe I missed something?

Copy link
Contributor Author

@quintesse quintesse Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the easiest reason would be that //DEPS already supported all these use-cases for quite a while. It's not a new feature that was added recently or anything, I was just fixing a bug in the existing code.

And on the personal end, I find it nicer to deal with a single //DEPS concept that can handle all cases without having to think about which directive I have to use in which case. I also don't think it's that important to be explicit about ordering, I'd say that in 99.9% of cases you don't care about ordering, and in the remaining case that you do you could read in the docs what the ordering will be. And finally I think the UX experience with a single directive is simply better than one with 3.

Edit: Now, I can certainly understand that there is a case to be made that JAR files are somewhat an outlier here, because they don't define dependencies themselves. But to introduce a separate directive just for them just doesn't seem worthwhile. Just like not all dependencies have (sub)dependencies of their own, you could see a JAR file as simply a dependency that never has its own dependencies.

ss.addClassPaths(DependencyUtil.filterJarDeps(directives.dependencies()));
ss.addCompileOptions(directives.compileOptions());
ss.addNativeOptions(directives.nativeOptions());
prj.addRepositories(directives.repositories());
Expand All @@ -362,10 +363,7 @@ private Project createJbangProject(ResourceRef resourceRef) {

ResourceResolver resolver = getAliasResourceResolver(null);
ResourceResolver sibRes2 = getSiblingResolver(resourceRef, resolver);
for (String srcDep : directives.sourceDependencies()) {
ResourceRef subRef = resolver.resolve(srcDep, true);
prj.addSubProject(new ProjectBuilder(buildRefs).build(subRef));
}
handleSourceDeps(DependencyUtil.filterSourceDeps(directives.dependencies()), resolver, prj);

boolean first = true;
List<String> sources = directives.sources();
Expand Down Expand Up @@ -397,6 +395,20 @@ private Project createJbangProject(ResourceRef resourceRef) {
return updateProject(prj);
}

private void handleSourceDeps(List<String> srcDeps, ResourceResolver resolver, Project prj) {
for (String srcDep : srcDeps) {
ResourceRef subRef = resolver.resolve(srcDep, true);
Project subPrj;
if (subRef != null) {
subPrj = new ProjectBuilder(buildRefs).build(subRef);
} else {
subPrj = new Project(ResourceRef.forUnresolvable(srcDep,
"Could not be resolved from " + resolver.description()));
}
prj.addSubProject(subPrj);
}
}

private Project createSourceProject(ResourceRef resourceRef) {
Source src = createSource(resourceRef);
Project prj = new Project(src);
Expand Down Expand Up @@ -657,7 +669,8 @@ private Project updateProject(Source src, Project prj, ResourceResolver resolver
}
ResourceResolver sibRes1 = getSiblingResolver(srcRef);
ss.addResources(allToFileRef(src.getDirectives().files(), srcRef, sibRes1));
ss.addDependencies(src.collectBinaryDependencies());
ss.addDependencies(DependencyUtil.filterGavDeps(src.collectDependencies()));
ss.addClassPaths(DependencyUtil.filterJarDeps(src.collectDependencies()));
ss.addCompileOptions(src.getCompileOptions());
ss.addNativeOptions(src.getNativeOptions());
prj.addRepositories(src.getDirectives().repositories());
Expand All @@ -680,10 +693,7 @@ private Project updateProject(Source src, Project prj, ResourceResolver resolver
prj.setJavaVersion(version);
}
}
for (String srcDep : src.collectSourceDependencies()) {
ResourceRef subRef = sibRes1.resolve(srcDep, true);
prj.addSubProject(new ProjectBuilder(buildRefs).build(subRef));
}
handleSourceDeps(DependencyUtil.filterSourceDeps(src.collectDependencies()), sibRes1, prj);
ResourceResolver sibRes2 = getSiblingResolver(srcRef, resolver);
List<Source> includedSources = allToSource(src.getDirectives().sources(), srcRef, sibRes2);
for (Source includedSource : includedSources) {
Expand Down Expand Up @@ -723,11 +733,7 @@ private ModularClassPath resolveDependency(String dep) {
if (mcp == null) {
DependencyResolver resolver = new DependencyResolver()
.addDependency(dep)
.addRepositories(allToMavenRepo(
replaceAllProps(additionalRepos)))
.addDependencies(replaceAllProps(additionalDeps))
.addClassPaths(
replaceAllProps(additionalClasspaths));
.addRepositories(allToMavenRepo(replaceAllProps(additionalRepos)));
mcp = resolver.resolve();
}
return mcp;
Expand Down
8 changes: 2 additions & 6 deletions src/main/java/dev/jbang/source/Source.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,12 +83,8 @@ protected String getContents() {

public abstract @NonNull Type getType();

protected List<String> collectBinaryDependencies() {
return getDirectives().binaryDependencies();
}

protected List<String> collectSourceDependencies() {
return getDirectives().sourceDependencies();
protected List<String> collectDependencies() {
return getDirectives().dependencies();
}

protected abstract List<String> getCompileOptions();
Expand Down
17 changes: 1 addition & 16 deletions src/main/java/dev/jbang/source/parser/Directives.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import org.jspecify.annotations.Nullable;

import dev.jbang.dependencies.DependencyUtil;
import dev.jbang.dependencies.JitPackUtil;
import dev.jbang.dependencies.MavenCoordinate;
import dev.jbang.dependencies.MavenRepo;
import dev.jbang.util.JavaUtil;
Expand Down Expand Up @@ -56,32 +55,18 @@ public abstract static class Names {

public abstract Stream<Directive> getAll();

public List<String> binaryDependencies() {
public List<String> dependencies() {
return getAll()
.filter(this::isDependDeclare)
.map(Directive::getValue)
.flatMap(v -> quotedStringToList(Q2TL_SSCT, v).stream())
.filter(Directives::isGav)
.collect(Collectors.toList());
}

public List<String> sourceDependencies() {
return getAll()
.filter(this::isDependDeclare)
.map(Directive::getValue)
.flatMap(v -> quotedStringToList(Q2TL_SSCT, v).stream())
.filter(it -> !isGav(it))
.collect(Collectors.toList());
}

protected boolean isDependDeclare(Directive d) {
return Names.DEPS.equals(d.getName());
}

private static boolean isGav(String ref) {
return DependencyUtil.looksLikeAPossibleGav(ref) || !JitPackUtil.ensureGAV(ref).equals(ref);
}

public List<MavenRepo> repositories() {
return getAll()
.filter(this::isRepoDeclare)
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/dev/jbang/source/sources/GroovySource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@ protected List<String> getRuntimeOptions() {
}

@Override
protected List<String> collectBinaryDependencies() {
final List<String> allDependencies = super.collectBinaryDependencies();
protected List<String> collectDependencies() {
final List<String> allDependencies = super.collectDependencies();
final String groovyVersion = getGroovyVersion();
if (groovyVersion.startsWith("4.") || groovyVersion.startsWith("5.")) {
allDependencies.add("org.apache.groovy:groovy:" + groovyVersion);
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/dev/jbang/source/sources/KotlinSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,8 @@ public KotlinSource(ResourceRef script, Function<String, String> replaceProperti
}

@Override
protected List<String> collectBinaryDependencies() {
final List<String> allDependencies = super.collectBinaryDependencies();
protected List<String> collectDependencies() {
final List<String> allDependencies = super.collectDependencies();
allDependencies.add("org.jetbrains.kotlin:kotlin-stdlib:" + getKotlinVersion());
return allDependencies;
}
Expand Down
6 changes: 3 additions & 3 deletions src/test/java/dev/jbang/cli/TestRun.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,7 @@ void testHelloWorldJar() throws IOException {

CommandLine.ParseResult pr = JBang.getCommandLine()
.parseArgs("run", "--deps", "info.picocli:picocli:4.6.3",
"--cp", "dummy.jar", jar);
"--cp", jar, examplesTestFolder.resolve("echo.java").toString());
Copy link
Collaborator

@maxandersen maxandersen Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this not changing meaning of the test?

Copy link
Contributor Author

@quintesse quintesse Oct 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little bit, but I changed it exactly to maintain what it's trying to test. In the new code adding a jar that doesn't exist will fail early. So keeping the test the same would actually fail to test what it's trying to test.

Run run = (Run) pr.subcommand().commandSpec().userObject();

ProjectBuilder pb = run.createProjectBuilderForRun();
Expand All @@ -357,7 +357,6 @@ void testHelloWorldJar() throws IOException {
assertThat(code.getMainClass(), not(nullValue()));

assertThat(result, containsString("picocli-4.6.3.jar"));
assertThat(result, containsString("dummy.jar"));
assertThat(result, containsString("hellojar.jar"));

assertThat(code.getResourceRef().getFile().toString(), equalTo(jar));
Expand Down Expand Up @@ -2175,9 +2174,10 @@ void testGAVCliReposAndDepsTwoRepos(@TempDir File output) throws IOException {
Run run = (Run) pr.subcommand().commandSpec().userObject();

ProjectBuilder pb = run.createProjectBuilderForRun();
Project prj = pb.build(jar);

try {
pb.build(jar);
run.updateGeneratorForRun(prj.codeBuilder().build()).build().generate();
fail("Should have thrown exception");
} catch (ExitException ex) {
StringWriter sw = new StringWriter();
Expand Down
20 changes: 20 additions & 0 deletions src/test/java/dev/jbang/source/TestBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import static org.hamcrest.CoreMatchers.endsWith;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.*;
import static org.junit.jupiter.api.Assertions.assertThrowsExactly;

import java.io.File;
import java.io.IOException;
Expand All @@ -30,6 +31,7 @@
import dev.jbang.catalog.Alias;
import dev.jbang.catalog.CatalogUtil;
import dev.jbang.cli.ExitException;
import dev.jbang.resources.ResourceNotFoundException;
import dev.jbang.source.buildsteps.IntegrationBuildStep;
import dev.jbang.source.buildsteps.JarBuildStep;
import dev.jbang.source.buildsteps.NativeBuildStep;
Expand Down Expand Up @@ -554,4 +556,22 @@ protected void runNativeBuilder(List<String> optionList) throws IOException {
}
}.get().build();
}

@Test
void testMissingJarDeps() {
ProjectBuilder pb = Project.builder();
Path src = examplesTestFolder.resolve("brokendeps_jar.java");
Project prj = pb.build(src);
BuildContext ctx = BuildContext.forProject(prj);
assertThrowsExactly(ResourceNotFoundException.class, ctx::resolveClassPath);
}

@Test
void testMissingSourceDeps() {
ProjectBuilder pb = Project.builder();
Path src = examplesTestFolder.resolve("brokendeps_source.java");
Project prj = pb.build(src);
BuildContext ctx = BuildContext.forProject(prj);
assertThrowsExactly(ResourceNotFoundException.class, ctx::resolveClassPath);
}
}
Loading
Loading