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.

Sign up for GitHub

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

Jump to bottom

Add clangtidy for gmake2 #2247

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add clangtidy for gmake2 #2247

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion modules/gmake2/gmake2_cpp.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,13 @@
fileExtension { ".cc", ".cpp", ".cxx", ".mm" }
buildoutputs { "$(OBJDIR)/%{file.objname}.o" }
buildmessage '$(notdir $<)'
buildcommands {'$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"'}
buildcommands {
-- code checking must be before the build, so that if the code checking
-- fails, the build is not completed, and hence, a re-run will trigger
-- the code check again.
'$(CLANG_TIDY) "$<" -- %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How difficult would it be to make it so this isn't emitted when clang-tidy isn't requested, rather than always emitting and just "ignoring" it sometimes?

Copy link
Contributor Author

@theComputeKid theComputeKid Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nickclark2016 : I knew someone would ask this question, so I already pre-empted the answer in my original PR description: Unfortunately, at the point where the rules are decided, we do not have access to the value of the clangtidy property. You can see it here in this function, there is no information to play with. And then we have other issues as well like turning clang tidy on/off depending on the project configuration.

Then again, I am an outsider to the premake team, you probably know more about the code and can suggest better.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From "rule" point of view, we would need

  • 2 rules: rule 'cpp' and rule 'cpp_with_clang_tidy'
  • remove the fileExtensions and do the dispatch "manually".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please give some psuedo code to explain what you are saying? I'm not sure I understand.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mix with customcommand as fileextension cannot be removed...
Idea is to customize rule to handle clangtidy:
if you read https://premake.github.io/docs/Custom-Rules/
we might add propertydefinition to handle clangtidy, then we have to activate that property for each filecfg with clangtidy.
Not sure at which point it is doable without hack.
I will try to add clang-tidy support to premake-ninja, but it doesn't use (premake) rule internally but has "intrinsic" rule.
Above (adapted) method should do the trick for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. This sounds very messy. Not sure if there is an acceptable solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like there is no elegant fix for adding clang tidy for gmake2. It was fun trying to get it to work but no worries, feel free to close this PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I succeeded with ninja, and failed with gmake currently (to have a correct per-file per-config)
Some features are already partially working (per-file not taken into account for some generator).
Might be better to have partial working solution than no solution at all...
BTW, I will see if I can improve your PR with my suggestion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-o "$@" -MF "$(@:%.o=%.d)" -c "$<" seems superfluous/wrong.

'$(CXX) %{premake.modules.gmake2.cpp.fileFlags(cfg, file)} $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"'
}

rule 'cc'
fileExtension {".c", ".s", ".m"}
Expand Down Expand Up @@ -326,6 +332,7 @@
cpp.cppFlags,
cpp.cFlags,
cpp.cxxFlags,
cpp.clangtidy,
cpp.resFlags,
cpp.libs,
cpp.ldDeps,
Expand Down Expand Up @@ -425,6 +432,13 @@
p.outln('ALL_CXXFLAGS += $(CXXFLAGS) $(ALL_CPPFLAGS)' .. flags)
end

function cpp.clangtidy(cfg, toolset)
if cfg.clangtidy ~= nil then
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Off" (or false if there is some translation phases for that parameter) would wrongly enter in that case.

p.outln('CLANG_TIDY = clang-tidy')
else
p.outln('CLANG_TIDY = \\#')
end
end

function cpp.resFlags(cfg, toolset)
local resflags = table.join(toolset.getdefines(cfg.resdefines), toolset.getincludedirs(cfg, cfg.resincludedirs), cfg.resoptions)
Expand Down
1 change: 1 addition & 0 deletions modules/gmake2/tests/_tests.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ require ("gmake2")
return {
"test_gmake2_buildcmds.lua",
"test_gmake2_clang.lua",
"test_gmake2_clangtidy.lua",
"test_gmake2_file_rules.lua",
"test_gmake2_flags.lua",
"test_gmake2_includes.lua",
Expand Down
45 changes: 45 additions & 0 deletions modules/gmake2/tests/test_gmake2_clangtidy.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
--
-- test_gmake2_clangtidy.lua
-- Test ClangTidy support in Makefiles.
--

local suite = test.declare("gmake2_clangtidy")
local gmake2 = premake.modules.gmake2

local wks, prj, cfg

function suite.setup()
wks = workspace("MyWorkspace")
configurations { "Debug", "Release" }
targetname "blink"
kind "StaticLib"
language "C++"
prj = test.createProject(wks)
end

local function prepare()
wks = test.getWorkspace(wks)
prj = test.getproject(wks, 1)
cfg = test.getconfig(prj, "Debug")
gmake2.cpp.clangtidy(cfg, toolset)
files { "src/hello.cpp", "src/hello2.c" }

end

function suite.clangtidyOn()
clangtidy "On"

prepare()

test.capture [[
CLANG_TIDY = clang-tidy
]]
end

function suite.clangtidyOff()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
function suite.clangtidyOff()
function suite.clangtidyDefaulted()

currently, and you might add a dedicated test for "Off", especially when code suggest that it is wrongly handled ;-)

prepare()

test.capture [[
CLANG_TIDY = \#
]]
end
5 changes: 5 additions & 0 deletions modules/gmake2/tests/test_gmake2_file_rules.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,9 +94,11 @@

$(OBJDIR)/hello.o: src/greetings/hello.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/hello1.o: src/hello.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"

]]
Expand All @@ -119,6 +121,7 @@ $(OBJDIR)/hello.o: src/hello.c
$(SILENT) $(CC) $(ALL_CFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/test.o: src/test.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"

]]
Expand All @@ -139,6 +142,7 @@ $(OBJDIR)/test.o: src/test.cpp

$(OBJDIR)/hello.o: src/hello.c
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/test.o: src/test.c
@echo "$(notdir $<)"
Expand Down Expand Up @@ -166,6 +170,7 @@ $(OBJDIR)/test.o: src/test.c
ifeq ($(config),debug)
$(OBJDIR)/hello.o: src/hello.c
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"

else ifeq ($(config),release)
Expand Down
6 changes: 6 additions & 0 deletions modules/gmake2/tests/test_gmake2_pch.lua
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,9 +70,11 @@

$(OBJDIR)/a.o: a.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/b.o: b.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
]]
end
Expand Down Expand Up @@ -194,9 +196,11 @@ PCH = ../../../../src/host/premake.h

$(OBJDIR)/a.o: a.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/b.o: b.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
]]
end
Expand All @@ -216,9 +220,11 @@ $(OBJDIR)/b.o: b.cpp

$(OBJDIR)/a.o: a.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(OBJDIR)/b.o: b.cpp
@echo "$(notdir $<)"
$(SILENT) $(CLANG_TIDY) "$<" -- -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
$(SILENT) $(CXX) -include $(PCH_PLACEHOLDER) $(ALL_CXXFLAGS) $(FORCE_INCLUDE) -o "$@" -MF "$(@:%.o=%.d)" -c "$<"
]]
end
4 changes: 2 additions & 2 deletions website/docs/clangtidy.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
Enables clang-tidy code analysis for Visual Studio.

The `clangtidy` option enables running clang-tidy code analysis in Visual Studio projects.
The `clangtidy` option enables running clang-tidy code analysis in Visual Studio and gmake2 projects.

```lua
clangtidy("value")
Expand All @@ -19,7 +19,7 @@ The `config` scope.

### Availability ###

Premake 5.0.0 beta 3 or later for Visual Studio 2019 and later.
Premake 5.0.0 beta 3 or later. Implemented for Visual Studio 2019 and later, and gmake2.

### See Also ###

Expand Down