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

Add functionality to exclude files that will not be tracked for changes. #1847

Closed
LeonovecSergey opened this issue Aug 19, 2021 · 6 comments · Fixed by #1848
Closed

Add functionality to exclude files that will not be tracked for changes. #1847

LeonovecSergey opened this issue Aug 19, 2021 · 6 comments · Fixed by #1848

Comments

@LeonovecSergey
Copy link
Contributor

LeonovecSergey commented Aug 19, 2021

  • The selection mechanism relies on VSCode watch changed files glob patterns. Currently, there is no possibility to use negations in these patterns, therefore all changed files on the workspace, including Bazel-build related, are sent to the language server.
  • In the case of Bazel-build folders, there is a symlink to an original module with corresponding sources and BUILD files. Every build refreshes the symlink and, therefore, the timestamp of this symlink is updated.
  • The updated timestamp leads to include these files in the list of changed ones and passed them to the language server.
  • On the language server side in the org.eclipse.jdt.ls.core.internal.managers.StandardProjectsManager#fileChanged method all paths, including symlinks, are converted into Eclipse-workspace relative paths by the JDTUtils#getFileOrFolder. Symlinks are transformed as the corresponding original files with the same Eclipse-workspace relative paths.
	@Override
	public void fileChanged(String uriString, CHANGE_TYPE changeType) {
...
		IResource resource = JDTUtils.getFileOrFolder(uriString);
		if (resource == null) {
			return;
		}
		try {
			Optional<IBuildSupport> bs = getBuildSupport(resource.getProject());
			if (bs.isPresent()) {
				IBuildSupport buildSupport = bs.get();
				boolean requireConfigurationUpdate = buildSupport.fileChanged(resource, changeType, new NullProgressMonitor());
...
  • IBuildSupport#fileChanged receives transformed paths only and it has no possibility to filter valid from invalid files, which results in a false prompt dialog on the client to rebuild the application.
@rgrunber
Copy link
Contributor

Ok, so this is essentially producing a build cycle (with prompt) because of the symlink updated after the build, combined with the loss of original path information (before symlink is resolved).

Can your implementation of IBuildSupport#getWatchPatterns() not be more selective about which files it watches ? For example, instead of including the symlink file in watchers, it could include the files/folders under that folder ?

@LeonovecSergey
Copy link
Contributor Author

As I mentioned earlier, negation cannot be used in these patterns. Both the project module folders and the symbolic link are in the same folder. It follows that to track BUILD files, you need to calculate the paths for each specific file.

We expect our plugin to be used for large projects. For example, we are currently testing a project that contains over 2000 modules.

So I think that calculating the path for each file for larger projects can take a long time.

@rgrunber
Copy link
Contributor

@fbricon , let me know what you think. The change looks fine to me (I would maybe just look closer at the glob: matching).

Is it worth seeing if getFileOrFolder(..) can be fixed to return the symlink rather than the resolved path ?

@jdneo
Copy link
Contributor

jdneo commented Sep 2, 2021

@LeonovecSergey Please let me know if I missed anything, is it possible in BazelBuildSupport#fileChanged() return false to avoid the prompt dialog?

From my understanding if BazelBuildSupport knows which kind of files need to be excluded, it should know whether this change needs to have an update. Is this correct?

Is it worth seeing if getFileOrFolder(..) can be fixed to return the symlink rather than the resolved path?

@rgrunber Personally I would suggest to wait to see if more customers complain about that, this utility exists there for more than 2 years. Changing the behavior might cause other unexpected regression.

@rgrunber
Copy link
Contributor

rgrunber commented Sep 2, 2021

@jdneo , the file they wish to exclude is a symbolic link to a file that should be watched by the build. It's also not possible to specify each and every file/folder that must be included. It seems like getFileOrFolder(..) resolves such files, losing the original location, and so they have no way to determine whether the resolved resource actually refers to the symlinked file. They certainly don't want to exclude the resolved file. As a result, I believe they can't exclude in fileChanged() given the current API.

Agreed. Changing getFileOrFolder(..) has the potential to cause some issues. I'm not as worried about this PR because nothing should be felt on our end at all. For now we wouldn't implement the method (getExcludedFilePatterns), and the default doesn't return true (to exclude) any file.

@jdneo
Copy link
Contributor

jdneo commented Sep 3, 2021

I'm not as worried about this PR because nothing should be felt on our end at all.

Yes, agree. Just leave some comments in the PR, overall looks good to me.

@rgrunber rgrunber added this to the Mid September 2021 milestone Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants