fix: missing source and jar deps now handled correctly

[quintesse]

Fixes #2251

[quintesse]

Simplified Directives somewhat by removing its ability to split dependencies into binary and source. It now simply returns what it finds in the source without applying any modifications. It's now up to the callers to do that.

Also changed info tools to show the dependencies as they appear in the source file, right now it doesn't actually show any jar and source dependencies. Jar and source dependencies would show up in resolvedDependencies but only if resolving was successful, if not, then there would not be any mention of those dependencies. With this change they'll always be added to dependencies. Also source dependencies will show up as subProjects which is a list of resource refs, so you'll be able to see what the backing file is or if there was an error resolving the file.

[maxandersen]

so we add gavs first, jars second?

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

[quintesse]

//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?

[quintesse]

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)

[maxandersen]

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 ? :)

[quintesse]

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 :-)

[quintesse]

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.

[maxandersen]

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.

[quintesse]

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

[maxandersen]

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?

[quintesse]

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.

[maxandersen]

is this not changing meaning of the test?

[quintesse]

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.

[maxandersen]

see comment

[quintesse]

Closing because it's not the way we want to do things.