-
Notifications
You must be signed in to change notification settings - Fork 161
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
base: main
Are you sure you want to change the base?
Conversation
List<String> srcDirs = project .getMainSourceSet() | ||
.getSources() | ||
.stream() | ||
.map(x -> x.getFile().getParent().toAbsolutePath()) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
a680143
to
28bdc4b
Compare
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
fyi, the jshell tests fails locally for me too thus there is something causing jshell code to fail. |
i.e.
so looks like needing to not set source path for jshell based launches? |
is this still relevant @quintesse ? |
@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. |
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