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

VsChromium causes build breaks with clang-cl - updated filters needed #31

Closed
randomascii opened this issue Aug 2, 2017 · 12 comments
Closed

Comments

@randomascii
Copy link

From a coworker's report:

If you build Chromium on Windows and run Visual Studio with VS-Chromium extension you may start seeing some seemingly random 'permission denied' errors during the build now that clang-cl is the default compiler.

This was investigated with PROCMON and it appears that the VS Chromium extension scans the source tree during the build and ends up reading temporary obj files created by clang.

Here are a few examples of files that can cause problems:
dc_layer_overlay.obj-3b0874b9,
V8ConstrainDoubleRange.obj-606e8091,
typed_arrays_cc.pch-232411ff.

Because VS-Chromium doesn't recognize the file extensions it doesn't realize that these are files that should be excluded from scanning so it scans them, thus briefly holding a lock on them, thus causing build failures.

The workaround is to modify vs-chromium-project.txt to exclude .obj and .pch instead of *.obj and *.pch. This is easy enough for those who already have a vs-chromium-project.txt file, but tricky for others since creating such a file overrides the default filters and therefore can cause new problems. A workable vs-chromium-project.txt file with these modifications is attached - drop that in src\chromium\src to reconfigure VS-Chromium.

Building in these new filters would be very helpful.

vs-chromium-project.txt

@rpaquay
Copy link
Contributor

rpaquay commented Aug 2, 2017

I am a little confused as your suggestion for a fix: Are you suggesting that by default (i.e. when there is no vs-chromium-project.txt file detected), VsChromium excludes .obj files?

This would mean changing this file: https://github.com/chromium/vs-chromium/blob/5242433fcde80254fcba65956139357076fe2bc9/src/Server/Configuration/SearchableFiles.ignore


I wonder if an additional fix would be the change the way VsChromium read files (see

var fileHandle = NativeMethods.CreateFile(fileInfo.FullPath.Value, NativeAccessFlags.GenericRead, FileShare.Read, IntPtr.Zero,
).

Currently, VsChromium uses "FileShare.Read", but it could as well use "FileShare.ReadWrite | FileShare.Delete", this should let other application (clang) delete files.

@randomascii
Copy link
Author

My suggestion was to change *.obj in src/Server/Configuration/SearchableFiles.ignore to .obj, and *.pch to .pch.

Looking at that file I can see other changes which should be made. Now that gn has replaced gyp the explicit references to Debug and Release directories don't make sense. It would be better to replace:

out/Debug/gen/webkit/bindings/
out/Release/gen/webkit/bindings/

with:

out/*/gen/webkit/bindings

except that that directory doesn't exist anymore so that I think what is actually wanted is:

out/*/gen/blink/bindings

@nico
Copy link

nico commented Aug 2, 2017

https://reviews.llvm.org/D36238 might address this as well.

@randomascii
Copy link
Author

That may be a much better fix.

@rpaquay
Copy link
Contributor

rpaquay commented Aug 2, 2017

Agreed, the proposed fix is probably better overall. Unrelated to VsChromium, but I can see, for example, people configuring their anti-virus to skip .obj and .pch files (by extension).

That said, it sounds like a cleanup of the default SearchableFiles.ignore file would not hurt.

@randomascii Would you have time to create a PR? Or maybe attach a new version of the file as a comment here so I can make the change?

In the mean time, I will make a fix to use the "FileShare.ReadWrite | FileShare.Delete" flags when opening files. Basically, VsChromium should not prevent other apps to do whatever they want to file, as VsChromium re-synchronizes its index anyways after file change notifications.

@rpaquay
Copy link
Contributor

rpaquay commented Aug 2, 2017

@randomascii Quick question: Didn't you add a vs-chromium-project.txt file to the Chromium repository? (Reason I am asking is that if there is one such file, the default file is ignored)

@randomascii
Copy link
Author

I added a vs-chromium-project.txt file to my repo, but I have not checked one in. Doing that is another possible idea, especially if we want to update the filters for the many changes that have happened in the last few years (gn mostly).

rpaquay added a commit that referenced this issue Aug 2, 2017
Fixes bug #31, or at least, greatly decrease the probability of
this happening.
rpaquay added a commit that referenced this issue Aug 2, 2017
rpaquay added a commit that referenced this issue Aug 2, 2017
@rpaquay
Copy link
Contributor

rpaquay commented Aug 2, 2017

Ok, I created a new pre-release (https://github.com/chromium/vs-chromium/releases/tag/0.9.22) that contains the update to skip clang temporary files, and also to avoid locking files when reading them.

Let me know if this works for you guys, I'll publish in a couple of days.

@randomascii
Copy link
Author

It looks like the new release fixes the issues. Thanks!

@rpaquay
Copy link
Contributor

rpaquay commented Aug 4, 2017

Ok, I just released v0.9.22 with this fix.

@rpaquay rpaquay closed this as completed Aug 4, 2017
@nico
Copy link

nico commented Aug 8, 2017

As of https://reviews.llvm.org/D36413 , clang now generates foo-12345.obj.tmp files. It'd be good if you could add *.tmp to src/Server/Configuration/SearchableFiles.ignore too.

@rpaquay
Copy link
Contributor

rpaquay commented Aug 8, 2017

It looks like *.tmp is already in src/Server/Configuration/SearchableFiles.ignore, so we should be good to go :)

(for reference: https://github.com/chromium/vs-chromium/blob/0.9.22/src/Server/Configuration/SearchableFiles.ignore#L141)

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

No branches or pull requests

3 participants