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.

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

fix: Fixes dependent source discovery #1508

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

quintesse
Copy link
Contributor

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 explicitly 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 #1502

@quintesse quintesse marked this pull request as ready for review November 7, 2022 16:25
List<String> srcDirs = project .getMainSourceSet()
.getSources()
.stream()
.map(x -> x.getFile().getParent().toAbsolutePath())
Copy link
Collaborator

Choose a reason for hiding this comment

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

if I read this right it is adding blindly folders parent - not taking into consideration the package hierarchy - thus this would add the wrong root afaics?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's what I realized too, it's also why it's still Draft 😅

But that unfortunately means that I need to rethink the Project handling. I might need to add the idea of source folders to it, which is somewhat unfortunate because it adds complexity which until now wasn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't like the idea that suddenly we will eb 100% depending on the output of a method (getJavaPackage()) with a very dubious implementation. So far we haven't used it for anything essential besides edit, but now we'd need it for every build invocation.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we do it on just one file per unique folder. To not require it run on multiple files again and again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not so much worried about performance but more that the extraction of the information might be ambiguous. We're just doing a very simple match on lines that start with "package" so even a block comment that has the word "package" might match and throw things off. That was something that never bothered me before because at most it would cause some minor problems with the edit command, but now it would affect the actual running of the code and any mistake would result in fatal errors. So I'm somewhat hesitant suddenly requiring this information.

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

fyi, the jshell tests fails locally for me too thus there is something causing jshell code to fail.

@maxandersen
Copy link
Collaborator

i.e.

jbang helloworld.jsh
Unexpected exception reading startup: java.lang.RuntimeException: java.lang.IllegalArgumentException: error: --source-path requires an argument
java.lang.RuntimeException: java.lang.IllegalArgumentException: error: --source-path requires an argument
	at jdk.compiler/com.sun.tools.javac.api.JavacTool.getTask(JavacTool.java:198)
	at jdk.compiler/com.sun.tools.javac.api.JavacTaskPool.getTask(JavacTaskPool.java:177)
	at jdk.jshell/jdk.jshell.TaskFactory.runTask(TaskFactory.java:206)
	at jdk.jshell/jdk.jshell.TaskFactory.parse(TaskFactory.java:140)
	at jdk.jshell/jdk.jshell.TaskFactory.parse(TaskFactory.java:238)
	at jdk.jshell/jdk.jshell.Eval.sourceToSnippets(Eval.java:197)
	at jdk.jshell/jdk.jshell.Eval.eval(Eval.java:133)
	at jdk.jshell/jdk.jshell.JShell.eval(JShell.java:493)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.processSource(JShellTool.java:3563)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.processSourceCatchingReset(JShellTool.java:1310)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.processInput(JShellTool.java:1208)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.run(JShellTool.java:1181)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.startUpRun(JShellTool.java:1148)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.resetState(JShellTool.java:1104)
	at jdk.jshell/jdk.internal.jshell.tool.JShellTool.start(JShellTool.java:938)
	at jdk.jshell/jdk.internal.jshell.tool.JShellToolBuilder.start(JShellToolBuilder.java:261)
	at jdk.jshell/jdk.internal.jshell.tool.JShellToolProvider.main(JShellToolProvider.java:120)
Caused by: java.lang.IllegalArgumentException: error: --source-path requires an argument
	at jdk.compiler/com.sun.tools.javac.file.BaseFileManager.handleOption(BaseFileManager.java:234)
	at jdk.jshell/jdk.jshell.MemoryFileManager.handleOption(MemoryFileManager.java:343)
	at jdk.compiler/com.sun.tools.javac.api.ClientCodeWrapper$WrappedJavaFileManager.handleOption(ClientCodeWrapper.java:265)
	at jdk.compiler/com.sun.tools.javac.main.Arguments.doProcessArgs(Arguments.java:390)
	at jdk.compiler/com.sun.tools.javac.main.Arguments.processArgs(Arguments.java:347)
	at jdk.compiler/com.sun.tools.javac.main.Arguments.init(Arguments.java:246)
	at jdk.compiler/com.sun.tools.javac.api.JavacTool.getTask(JavacTool.java:185)
	... 16 more
Caused by: com.sun.tools.javac.main.Option$InvalidValueException: error: --source-path requires an argument
	at jdk.compiler/com.sun.tools.javac.main.OptionHelper.newInvalidValueException(OptionHelper.java:95)
	at jdk.compiler/com.sun.tools.javac.main.Option.handleOption(Option.java:1081)
	at jdk.compiler/com.sun.tools.javac.file.BaseFileManager.handleOption(BaseFileManager.java:232)
	... 22 more

so looks like needing to not set source path for jshell based launches?

@maxandersen
Copy link
Collaborator

is this still relevant @quintesse ?

@quintesse
Copy link
Contributor Author

@maxandersen well the original issue is still open and this PR is at least an intent towards a solution. But I haven't looked at this since last time it was discussed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic source discovery fails if script contains //DEPS
2 participants