GradleFiles project scan should adhere to global root settings#9263
Open
sid-srini wants to merge 2 commits intoapache:masterfrom
Open
GradleFiles project scan should adhere to global root settings#9263sid-srini wants to merge 2 commits intoapache:masterfrom
sid-srini wants to merge 2 commits intoapache:masterfrom
Conversation
Fixed GradleFiles scan for project files to honour the global scan root settings, if any. - Similar to that in projectapi SimpleFileOwnerQueryImpl. - GradleFiles needs to necessarily traverse the parent directory hierarchy to find the root project settings. - Further, the project detection needs to give precedence to maven projects also defined for the same directory, which has a similar hierarchy traversal requirement introduced in apache#1280. Fixed GradleFiles to find parentScript for kotlin script. - Added a unit test case for it in GradleFilesTest. Fixed SimpleFileOwnerQueryImplementation to check for scan root dirs without allowing sibling dirs with the same name prefix. - Example: if scan root = "/tmp/app", - then "/tmp/application" should not be allowed for scan; - while "/tmp/app" and "/tmp/app/src" should remain allowed. Fixed GradleFilesTest for File comparison on MacOS. - Needed due to the presence of "/private" prefix in the canonical path of unix temp folders. Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
mbien
added
Gradle
Split the class into 2 classes:
- GradleFilesWithScanRootTest now only runs the GradleFilesTest
with the project limitScanRoot and forbiddenFolders
properties set.
- GradleFilesScanRootTest contains only the functionality of the
scan-root and forbidden folders specifically.
- Only a single JUnit @test can be used here since the static
initialization in @before must be done once only.
- This means the test root folder must not change between tests.
- The actual gradle project is now a grandchild in the tree of the
test root dir.
- The original failure was due to the alternative placement of the
tests temporary folder on linux which is not sufficiently nested
within the OS tmp directory.
Signed-off-by: Siddharth Srinivasan <siddharth.srinivasan@oracle.com>
Contributor
Author
|
Hi @lkishalmi. Please review this when you get a chance. Thank you. |
Contributor
matthiasblaesing
left a comment
matthiasblaesing
left a comment
There was a problem hiding this comment.
In general makes sense to me. I left a few inline comments.
| for (String scanRoot : projectScanRoots) { | ||
| if (path.startsWith(scanRoot) | ||
| && (path.length() == scanRoot.length() | ||
| || path.charAt(scanRoot.length()) == '/')) |
Contributor
There was a problem hiding this comment.
I assume this change intents to check that we don't get a substring match for the last component. So far so sensible, but will it work on windows paths?
PS: Also does not match logic in GradleFiles.Searcher#notWithinProjectScanRoots
Comment on lines
+44
to
+49
| if (root != null && Utilities.isMac()) { | ||
| String absolutePath = root.getAbsolutePath(); | ||
| if (absolutePath.startsWith("/private/")) { | ||
| return new File(absolutePath.substring(8)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This needs a comment. What is so special about the temp on mac and why does this break anything?
| return isProjectCheck(parent, preferMaven); | ||
| FileObject parent = dir.getParent(); | ||
| if (parent != null && GradleFiles.Searcher.searchPath(FileUtil.toFile(parent), "pom.xml") != null) { // NOI18N | ||
| return isProjectCheck(parent, false); |
Contributor
There was a problem hiding this comment.
is preferMaven intentionally dropped here?
|
|
||
| Set<String> paths = null; | ||
| for (String split : joinedPaths.split(pathSeparator)) { | ||
| if ((split = split.trim()).isEmpty()) continue; |
Contributor
There was a problem hiding this comment.
This will break when path contains leading or trailing spaces. Or is this just a copy from SimpleFileOwnerQueryImplementation? If so, please mark both places that they need to be synced on change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixed
GradleFilesscan for project files to honour the global scan root settings, if any.SimpleFileOwnerQueryImpl.GradleFilesneeds to necessarily traverse the parent directory hierarchy to find the root project settings.Fixed
GradleFilesto findparentScriptfor kotlin script.GradleFilesTest.Fixed
SimpleFileOwnerQueryImplementationto check for scan root dirs without allowing sibling dirs with the same name prefix.Fixed
GradleFilesTestfor File comparison on MacOS.^Add meaningful description above
Click to collapse/expand PR instructions
By opening a pull request you confirm that, unless explicitly stated otherwise, the changes -
Please make sure (eg.
git log) that all commits have a valid name and email address for you in the Author field.If you're a first time contributor, see the Contributing guidelines for more information.
If you're a committer, please label the PR before pressing "Create pull request" so that the right test jobs can run.
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)