Skip to content
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

Jump to bottom

Automatic source discovery fails if script contains //DEPS #1502

Open
fbricon opened this issue Nov 3, 2022 · 7 comments · May be fixed by #1508
Open

Automatic source discovery fails if script contains //DEPS #1502

fbricon opened this issue Nov 3, 2022 · 7 comments · May be fixed by #1508
Labels
bug Something isn't working

Comments

@fbricon
Copy link
Contributor

fbricon commented Nov 3, 2022 •
edited
Loading

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 fbricon added the bug Something isn't working label Nov 3, 2022
@quintesse
Copy link
Contributor

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

@quintesse
Copy link
Contributor

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)

@fbricon
Copy link
Contributor Author

fbricon commented Nov 3, 2022

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

@maxandersen
Copy link
Collaborator

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
Copy link
Collaborator

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?

quintesse added a commit to quintesse/jbang that referenced this issue Nov 7, 2022
@quintesse
fix: Fixes dependent source discovery
80fc299 
Parent directories for sources are now added to the compiler's source
path so any types referenced in them that are available in the same
directories can be found even when they are not explicitely mentioned
in any `//SOURCES` lines. This also fixes the problem where adding a
`//DEPS` line would cause the compiler to be unable to find those same
types.

Fixes jbangdev#1502
@quintesse quintesse linked a pull request Nov 7, 2022 that will close this issue
Draft
quintesse added a commit to quintesse/jbang that referenced this issue Nov 7, 2022
@quintesse
fix: Fixes dependent source discovery
b591089 
Parent directories for sources are now added to the compiler's source
path so any types referenced in them that are available in the same
directories can be found even when they are not explicitely mentioned
in any `//SOURCES` lines. This also fixes the problem where adding a
`//DEPS` line would cause the compiler to be unable to find those same
types.

Fixes jbangdev#1502
@quintesse
Copy link
Contributor

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
Copy link
Contributor

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 added a commit to quintesse/jbang that referenced this issue Nov 21, 2022
@quintesse
fix: Fixes dependent source discovery
a680143 
Parent directories for sources are now added to the compiler's source
path so any types referenced in them that are available in the same
directories can be found even when they are not explicitely mentioned
in any `//SOURCES` lines. This also fixes the problem where adding a
`//DEPS` line would cause the compiler to be unable to find those same
types.

Fixes jbangdev#1502
quintesse added a commit to quintesse/jbang that referenced this issue Nov 21, 2022
@quintesse
fix: Fixes dependent source discovery
28bdc4b 
Parent directories for sources are now added to the compiler's source
path so any types referenced in them that are available in the same
directories can be found even when they are not explicitely mentioned
in any `//SOURCES` lines. This also fixes the problem where adding a
`//DEPS` line would cause the compiler to be unable to find those same
types.

Fixes jbangdev#1502
quintesse added a commit to quintesse/jbang that referenced this issue Nov 21, 2022
@quintesse
fix: Fixes dependent source discovery
0ec418b 
Parent directories for sources are now added to the compiler's source
path so any types referenced in them that are available in the same
directories can be found even when they are not explicitely mentioned
in any `//SOURCES` lines. This also fixes the problem where adding a
`//DEPS` line would cause the compiler to be unable to find those same
types.

Fixes jbangdev#1502
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Fixes dependent source discovery quintesse/jbang
3 participants
@maxandersen @fbricon @quintesse