Closed garymm closed 10 months ago
Hey, Gary! Great to hear from you again. Thanks for being the type of person that looks to give back, leaving things better than you found them. I really appreciate your work.
A few quick things I should ask before merging: 1) What do you think about having the tool just auto-strip those nvcc-specific flags? Seems like an opportunity to save people configuration time by automatically doing the right thing for them--and also to avoid the issue where the project-level config doesn't work out of tree (I think). The code would go in _all_platform_patch assuming it's a quick line or two. (some good examples there, too). Or it could go in a new _cuda_platform_patch if you think that's wiser. 2) Since it sounds like you're actively developing with CUDA (and I'm not at the moment), I should rely on you to make this good, rather than doing a bunch of testing myself. You've got comparative advantage and seem very competent. Happy to be here for questions, but please do chase down loose any ends if you haven't already done so, and then tell me, so I can merge with confidence. A couple example questions on my mind as I'm reading: Does NVCC need any other changes to _get_headers_gcc because its dependency-getting flags differ? For example, does the -MF parsing code need any adaptations for us to discover cached dependency files for CUDA? (Could be failing silently, I'm realizing.) Do we need to strip out any pre-existing cuda-specific dependency-file-generation flags that could interfere with ours? Etc. Totally possible you've already chased down all these and more--but please do and let me know :)
Thanks again, Chris
Thanks! Worth a quick skim (and set-subtract) of the documented flags to save future back-and-forth?
There are a lot of flags. My preference would be to start with this relatively small set, and then if you find that people are frequently needing to add more, I could write a script to try to automatically generate the set of flags that nvcc accepts that clang doesn't. But it's your repo so I can try to be more comprehensive now if you think it's best.
Happy to go with your call, but I'd probably go with the quick script, doubly if you're already finding you needed the more above?
I'm also realizing we'd probably better strip these flags after we run the header search with _get_files (just move it down below), since otherwise we might be removing or swapping important flags needed for the header search to succeed. Does that check with your understanding? Are you seeing any issues like that?
Still do want to ask you to check in on those other things above (like hitting bazel's .d header cache and stripping existing flags--and just generally your giving it a close look to making sure things are working as expected and as good as they could be).
How's clangd's handling of the cuda files more generally? I'm hoping decent, since clang can do some cuda compiling IIRC, but you tell me.
Cheers and thanks for contributing back! Chris
Happy to go with your call, but I'd probably go with the quick script, doubly if you're already finding you needed the more above?
Added the script.
I'm also realizing we'd probably better strip these flags after we run the header search with _get_files
Done.
Still do want to ask you to check in on those other things above (like hitting bazel's .d header cache and stripping existing flags--and just generally your giving it a close look to making sure things are working as expected and as good as they could be).
It looks like rules_cuda
doesn't currently construct commands that construct the header cache. I could try to pre-emptively support it but then I'm not entirely sure it would work if/when rules_cuda
does implement it.
If / when they do it, they could just use the same args as are used for GCC or clang, which is what they'll probably do, so it will probably just work.
How's clangd's handling of the cuda files more generally? I'm hoping decent, since clang can do some cuda compiling IIRC, but you tell me.
It seems pretty good so far, but actually I'm intentionally restricting almost all my code to be compilable for both CPU and CUDA so there may very well be CUDA features that clangd chokes on.
Still valid but closing since it's been ignored for so long.
Darn. I'm so sorry I dropped this, Gary. Really appreciate all your great work--just got totally buried and missed the notification.
I'd love to still merge; sounds like you're ok with that.
Merged--and pushed to HEAD, along with some tweaks and bug-head-offs I saw while reading closely.
I'd really appreciate your giving it a whirl and a take a peek at the commits--and if I broke anything or otherwise made anything worse, please holler or toss up PRs (that I'll merge promptly this time).
[Github isn't recognizing, so I'm going to mark as closed, but your commits are rebased and applied :) Don't worry. Look at that shiny contributor badge!]
Thanks again, Gary, for being the type of person that looks to give back, leaving this better than you found it--and for all your great work and thinking here. I like your style and appreciate your great competence.
Thanks for the kind words. Thanks for maintaining this repo. I'll open another PR if I find any issues with it.
Thanks so much, man.
Great work, now I can retire my .clangd all around my repos.
The approach is to modify nvcc commands so as to make it compatible with clang's command line interface.