Conversation
|
Setup my UTs for that one https://github.com/Jarod42/premake-sample-projects/actions/runs/19731306147/job/56532783308 It fails for following test projects: KO: In addition, the second consecutive build didn't trigger a "nothing to do" from ninja, but |
|
Is this based on the existing https://github.com/jimon/premake-ninja or from scratch implementation? |
|
@tritao: I would say scratch implementation from the code. |
| return msc.tools[tool] | ||
| end | ||
|
|
||
|
|
There was a problem hiding this comment.
Changes from this file seems unrelated to ninja
There was a problem hiding this comment.
Needed to unify tooling fetches from Ninja
There was a problem hiding this comment.
It seems to come from merge (conflict?) resolution.
Shown diff seems to be space/EOL change, or reordering only.
99% from scratch |
17000e7 to
ff541b6
Compare
ff541b6 to
753c49c
Compare
2467c7a to
5270987
Compare
| _p("") | ||
| end | ||
|
|
||
| function m.prebuildcommandsrule(cfg, toolset) |
There was a problem hiding this comment.
Is there reason that "rule" is singular for this one whereas most of other use plural?
There was a problem hiding this comment.
Just an oversight here. Will fix.
3773922 to
fce9cc6
Compare
1bc1de2 to
7a9dd91
Compare
modules/ninja/ninja_cpp.lua
Outdated
| _p("rule prelink") | ||
| _p(" command = $prelinkcommands") | ||
| -- Use shell wrapper based on target system's shell to ensure proper command execution | ||
| local shell = _OPTIONS.shell or iif(cfg.system == p.WINDOWS, "cmd", "posix") |
There was a problem hiding this comment.
Above you use os.shell(), but here and below you don't?
There was a problem hiding this comment.
Debugging code. This is gonna be backed out in a future change.
| --- | ||
|
|
||
| -- | ||
| -- Check that prebuild commands use cmd /c on Windows |
There was a problem hiding this comment.
This comment doesn't align with the test.
| --- | ||
|
|
||
| -- | ||
| -- Check that prelink commands use cmd /c on Windows |
| --- | ||
|
|
||
| -- | ||
| -- Check that postbuild commands use cmd /c on Windows |
There was a problem hiding this comment.
Yeah, these tests used to do that. Missed updating the comments
|
|
||
| -- The command should have quotes around paths | ||
| local captured = premake.captured() | ||
| test.istrue(captured:find('copy /B /Y', 1, true) ~= nil) |
There was a problem hiding this comment.
This doesn't appear to be testing what the test name and the comments suggest is being tested.
fce9cc6 to
a73d125
Compare
| end | ||
| end | ||
|
|
||
| function m.defaultTarget(wks) |
There was a problem hiding this comment.
That might handle https://premake.github.io/docs/startproject/
|
|
||
| -- The custom build should have the dependencies phony as an implicit dependency | ||
| test.capture [[ | ||
| build obj/Debug/App/data.cpp: custom data.in | bin/Debug/App.dependencies |
There was a problem hiding this comment.
As I understand, you create extra file for dependencies (not sure why), should it goes in 'obj' instead of 'bin' directory?
There was a problem hiding this comment.
Yeah, good call. That should probably go in there. I can probably rework it to get rid of the file itself and use phonies here
| test.capture [[ | ||
| build bin/Debug/ProjectC: link obj/Debug/ProjectC/c.o | bin/Debug/ProjectA bin/Debug/ProjectB | ||
| ldflags = $ldflags_ProjectC_Debug | ||
| ]] |
There was a problem hiding this comment.
Should we depend on output or the phony Project_Debug which would include postbuild action?
|
Closing to fix more issues, get it in a better state before review. |
What does this PR do?
Implements a Ninja exporter module. Supports C and C++, as well as custom build events.
How does this PR change Premake's behavior?
No breaking changes
Anything else we should know?
Sorry in advance
Did you check all the boxes?
closes #XXXXin comment to auto-close issue when PR is merged)You can now support Premake on our OpenCollective. Your contributions help us spend more time responding to requests like these!