Improve the test for clangd --check to choose files deterministically
#161072
Conversation
It seems that not all files in the Flutter repository are compatible with clangd. This change improves the clangd invocation in CI to choose files that are known to work with clangd.
8570c18 to
f09e364
Compare
matanlurey
left a comment
There was a problem hiding this comment.
Seems reasonable to me, thanks. LGTM.
|
auto label is removed for flutter/flutter/161072, due to This PR has not met approval requirements for merging. The PR author is not a member of flutter-hackers and needs 1 more review(s) in order to merge this PR.
|
|
@gaaclarke Can you give this a second look (and LGTM/auto-submit if LGTY)? Thanks! |
| for (final Object? entry in compileCommands) { | ||
| if (entry is Map<String, Object?>) { | ||
| final String? file = entry['file'] as String?; | ||
| if (file != null && file.endsWith('_fl__fl_assets_fixtures.cc')) { |
There was a problem hiding this comment.
This is fragile. What is the compile_commands.json look like for you? Instead can we check that the entry has 'command' 'directory' and 'file' keys?
There was a problem hiding this comment.
We are invoking clangd in this code, and previously, we had no verification at all. While adding more verification is a good idea in general, I don't think it's necessary in this case. Here's why:
- GN emits
directory,file, andcommandkeys, and we are using GN to generatecompile_commands.json. - LLVM explicitly emits an error if any of these keys (
file,command, ordirectory) are missing.
I don't think anyone would break such a well-known format. Given this, I believe the current implementation is sufficient, and additional verification would be redundant. Furthermore, we pin all dependencies, including GN and LLVM, so if anything goes wrong, our CI is designed to catch it.
There was a problem hiding this comment.
There are 3 assumptions baked into this:
- gn's output won't change -- You've mentioned this is unlikely to change, except this changing has predicated this whole PR.
_fl__fl_assets_fixtures.cccontinues to be compiled.- No file is added that is sorted before
_fl__fl_assets_fixtures.cc.
We should be able to eliminate these, there's no reason to bury a landmine.
There was a problem hiding this comment.
Thanks for raising these points. Let me clarify:
- The fields (
file,command,directory) incompile_commands.jsonare specified in the standard and are unlikely to change. Even if something does change, our CI pipeline is designed to detect such issues. - The order of files in
compile_commands.jsonis irrelevant for this implementation because it is an array of objects, and the code explicitly selects the first occurrence of_fl__fl_assets_fixtures.cc. This ensures that file order won't cause issues, which is the primary goal of this PR. _fl__fl_assets_fixtures.ccwas chosen because it has been implicitly used in prior implementations, making it a reasonable default.
That said, if you believe _fl__fl_assets_fixtures.cc is not the best choice, I'd appreciate your suggestions for an alternative file that:
- is guaranteed to compile, and
- is stable enough to avoid being renamed or removed in the future.
If you don't have a preference, I can switch to using flutter/fml/thread.cc as an alternative, which I believe meets the criteria.
I hope this clarifies things. Let me know if you have additional thoughts!
There was a problem hiding this comment.
My recommendation is to not look for any name. If you show me what your compile_commands.json looks like in a gist I can make a recommendation. I am suggesting looking for the entry that has file command and directory instead of looking for something with a specific filename.
There was a problem hiding this comment.
Thank you for your suggestion. Here are two examples of compile_commands.json files generated by GN:
These files differ only in their order but are otherwise identical. Please note that they are approximately 17MB each and can be automatically generated if you're building the Flutter engine from source.
My goal remains the same: to select a file that is stable enough to be tested with clangd, ensuring the test doesn’t block progress. Does this clarify my intention? If not, I’d be happy to elaborate further.
There was a problem hiding this comment.
Also, note that the order of entries might vary depending on the operating system, STL, or compiler version used.
There was a problem hiding this comment.
Those gist links give me a 404, can you fix them please?
There was a problem hiding this comment.
matanlurey
left a comment
There was a problem hiding this comment.
Needs more work. See Aaron's comment.
|
@gaaclarke Can you please take another look at the updated gist links please? |
|
This looks like it is stalled on a resolution to how we should deal with the compile_commands.json format. Is there any guidance on unblocking this since we are stuck on an old version of GN. |
|
Ping @jonahwilliams This is blocking the GN roll which is stuck on an ancient version of GN that lacks many debugging tools. |
jonahwilliams
left a comment
There was a problem hiding this comment.
LGTM
I think this is a reasonable change for us to unblock gn rolls. We can continue the discssion on the fragility of the clangd check in an issue (if its important).
|
autosubmit label was removed for flutter/flutter/161072, because - The status or check suite Linux linux_fuchsia_tests has failed. Please fix the issues identified (or deflake) before re-applying this label. |
It seems that not all files in the Flutter repository are compatible with clangd. This change improves the clangd invocation in CI to choose files that are known to work with clangd.
Reason for the change:
In #161012, I'm trying to update GN to the latest version. However, this change made the order of files in the compile_commands.json. This caused the clangd check to fail because
engine/src/flutter/tools/clangd_check/bin/main.dartcurrently chooses the first file in the compile_commands.json, and the newly chosen file (engine/src/flutter/third_party/skia/modules/skparagraph/src/Decorations.cpp) is not compatible with clangd. To fix this, I'm making the test choose the file that is known to work with clangd. (gen/flutter/assets/_fl__fl_assets_fixtures.cc)Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.