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

[GH-6970] Adding a system property to disable the multi source file handling. #6974

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

lahodaj
Copy link
Contributor

@lahodaj lahodaj commented Jan 18, 2024

I think the caption says it all. Not only won't the source root be registered, they won't even be computed by default. Which should basically eliminate the effect of the multi source root feature.

Relates to #6970

@lahodaj lahodaj added the ci:all-tests [ci] enable all tests label Jan 18, 2024
@lahodaj lahodaj added this to the NB21 milestone Jan 18, 2024
@matthiasblaesing
Copy link
Contributor

matthiasblaesing commented Jan 18, 2024

If I'm not missing something obvious, this will enable the multi source handling by default.

Property is set to true:

SETTING_EnableMultiSourceRoot=true

that value is compared to the string "true", which yields the boolean true. That is then inverted yielding boolean false.

public static boolean DISABLE_MULTI_SOURCE_ROOT = !"true".equals(Bundle.SETTING_EnableMultiSourceRoot());

So effectively nothing changed.

Edit: I overlooked the obvious ....

@matthiasblaesing
Copy link
Contributor

Less radical suggestion: #6975

Comment on lines 70 to 75
@Messages({
"SETTING_EnableMultiSourceRoot=false"
})
public class MultiSourceRootProvider implements ClassPathProvider {

public static boolean DISABLE_MULTI_SOURCE_ROOT = false;
public static boolean DISABLE_MULTI_SOURCE_ROOT = !"true".equals(Bundle.SETTING_EnableMultiSourceRoot());
Copy link
Member

Choose a reason for hiding this comment

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

@matthiasblaesing this is the NB default which is inverted false -> disable is set to true.

the property is the branding for the LSP server which I suppose is for the VSCode use case (which enables it by default).

this is how I understand this at least - I could be wrong.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah crap - yeah - overlooked that

Copy link
Member

Choose a reason for hiding this comment

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

Does this work correctly if someone translates the messages?
Should we also add "true" to @Messages?

Copy link
Member

Choose a reason for hiding this comment

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

@junichi11 not everything which is in property files is a user facing String. This is just a field which can be true or false - thats it. This is not going to be translated.

@Messages is just a mechanism to pull some properties into the code, since maintaining property files can be error prone.

@junichi11
Copy link
Member

junichi11 commented Jan 18, 2024

We should use [GITHUB-****] instead of [NETBEANS-****] like Matthias because [NETBEANS-****] means an issue of JIRA.

@neilcsmith-net
Copy link
Member

We should use [GITHUB-] instead of [NETBEANS-] like Matthias because [NETBEANS-****] means an issue of JIRA.

Most usage has been [GH-****] However, while this is being pointed out, the most useful thing is to make sure the issue is linked in the description. I've edited here. Issue references from titles don't work get automatically cross referenced.

@mbien
Copy link
Member

mbien commented Jan 21, 2024

even if this isn't merged as-is, a slightly modified version of this could be useful as additional safety net to #6975

feature would be enabled by default but could be disabled via -D system property if something shows up post-release. No need to hurry since this can be merged later IMO due to the fact that its trivial to move a constant into a property.

@neilcsmith-net neilcsmith-net added the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 22, 2024
@neilcsmith-net
Copy link
Member

Am inclined to agree with @mbien to have a kill switch via system property. Less inclined to agree with merging later - would be better in rc2 IMO.

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 22, 2024

Ok. I'll work on changing this to a system property. Thanks!

@lahodaj lahodaj force-pushed the NETBEANS-6970 branch 2 times, most recently from 24a91cf to f94483f Compare January 22, 2024 15:31
@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 22, 2024

Changed to a system property. To disable the multi source file handling, use: -J-Djava.disable.multi.source.root=true.

@neilcsmith-net
Copy link
Member

Assuming you meant -J-Djava.disable.multi.source.root=true?! Looks good to me. Will wait for tests and to see if anyone else has any comments. Thanks!

@lahodaj
Copy link
Contributor Author

lahodaj commented Jan 22, 2024

Assuming you meant -J-Djava.disable.multi.source.root=true?! Looks good to me. Will wait for tests and to see if anyone else has any comments. Thanks!

Right. Copied from the wrong command line.

@mbien mbien changed the title [NETBEANS-6970] Disabling the multi source handling by default. [NETBEANS-6970] Adding a system property to disable the multi source file handling. Jan 22, 2024
Copy link
Member

@mbien mbien left a comment

Choose a reason for hiding this comment

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

looks good, thanks! updated PR title.

@neilcsmith-net neilcsmith-net removed the do not merge Don't merge this PR, it is not ready or just demonstration purposes. label Jan 23, 2024
@neilcsmith-net neilcsmith-net changed the title [NETBEANS-6970] Adding a system property to disable the multi source file handling. [GH-6970] Adding a system property to disable the multi source file handling. Jan 23, 2024
@neilcsmith-net neilcsmith-net merged commit 3aba8c6 into apache:delivery Jan 23, 2024
39 checks passed
@mbien mbien added the Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form) label Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:all-tests [ci] enable all tests Java [ci] enable extra Java tests (java.completion, java.source.base, java.hints, refactoring.java, form)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants