jbang icon indicating copy to clipboard operation
jbang copied to clipboard

Automatic source discovery fails if script contains //DEPS

Open fbricon opened this issue 2 years ago • 7 comments

Describe the bug Given a Foo.java script and a companion Bar.java at the same level and no explicit //SOURCES Bar.java directive in Foo.java, running jbang Foo.java will yield different results depending on the presence of //DEPS directive or no.

To Reproduce Steps to reproduce the behavior:

  1. Have a Bar.java file containing
public class Bar{}
  1. Have a Foo.java file in the same directory, containing
///usr/bin/env jbang "$0" "$@" ; exit $?
public class Foo extends Bar {
    public static void main(String... args) {
        System.out.println("hello JBang");
    }
}
  1. Running jbang Foo.java yields:

[jbang] Building jar... hello JBang

  1. Now just add a //DEPS directive to Foo.java, like:
///usr/bin/env jbang "$0" "$@" ; exit $?
//DEPS com.github.lalyos:jfiglet:0.0.9
public class Foo extends Bar {
    public static void main(String... args) {
        System.out.println("hello JBang");
    }
}
  1. Running jbang Foo.java now yields:

[jbang] Building jar... /Users/fbricon/Dev/souk/jbangmain/Foo.java:4: error: cannot find symbol public class Foo extends Bar { ^ symbol: class Bar 1 error [jbang] [ERROR] Error during compile [jbang] Run with --verbose for more details

Expected behavior JBang should be able to compile the same file, regardless whether //DEPS directives are present or not.

JBang version

[jbang] jbang version 0.99.1 Cache: /Users/fbricon/.jbang/cache Config: /Users/fbricon/.jbang Repository:/Users/fbricon/.m2/repository 0.99.1

Additional context Adding //SOURCES Bar.java prevents compilation failures when //DEPS is present

fbricon avatar Nov 03 '22 13:11 fbricon

Wow, that's definitely a nasty regression. Thanks for reporting that.

quintesse avatar Nov 03 '22 13:11 quintesse

Hmm actually, this is not a regression, it seems it was always like this.

It's a behavior of the javac compiler I wasn't aware of: javac looks for sources in the classpath!

So when there's only a a Foo.java and a Bar.java the compiler is run like this: javac Foo.java and it will automatically pick up Bar.java.

But the moment you add a dependency the compiler invocation turns into this: javac --classpath /home/user/.m2/repository/some/artifact.jar Foo.java and suddenly the compilation fails because the compiler can't find Bar.java anymore.

If I manually add . to the classpath things start working like before again.

What's the behavior that we want here @maxandersen ? Should we allow auto-pickup of local sources or should we require explicit //SOURCES lines? (I'm assuming the former but I just want to confirm with you)

quintesse avatar Nov 03 '22 14:11 quintesse

Autopickup sources is the path to least surprises. Without it, IDEs would need to report errors as soon as //DEPS is added, which puts a lot of burden on the tooling

fbricon avatar Nov 03 '22 15:11 fbricon

nice analysis!

Basically what you are saying is that it works like javac was designed ...that default classpath is current directory...

But adding . on every classpath feels wrong too.

maxandersen avatar Nov 03 '22 15:11 maxandersen

jbang --cp . Foo.java works

What is probably more right is to explore using javac --source-path consistently.

i.e. add a --source-path for every identified "source root" path.

WDYT?

maxandersen avatar Nov 03 '22 15:11 maxandersen

Btw, this problem is actually not related to //DEPS at all. It's just that coincidentally if your code is in the default package the default class path of . just happens to be the same as the source folder. The moment the code uses a package it will always fail. This has actually never worked!

quintesse avatar Nov 07 '22 17:11 quintesse

So there's two additional issues here @maxandersen :

  • if you depend on the compiler automatically picking up files then you can't run Foo.java if that file is remotely located. Obviously Bar.java will never be found.
  • if that remote Foo.java has a package statement it will not be located in the correct directory. We'd first have to download it, test if it has a package statement and then relocate the file to a properly named folder.

Seems to me then that we'd have to treat local source files differently than remote ones: we'd do the package detection and adding the source folders for local files but not for remote files. Is that how you see it as well?

quintesse avatar Nov 21 '22 17:11 quintesse