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

Adds a new toolset driver flag to signal driver compatibility. #2260

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

Conversation

tritao
Copy link
Contributor

@tritao tritao commented Sep 25, 2024

This allows external consumers of those toolsets to know what kind of driver the tool uses.

This can be important in some cases, like in Ninja, which provides some dependency handling features that need to change depending if the compiler is GCC or MSVC-like.

The problem is that due to lack of this information, the Ninja generator ends up having hardcoded logic for this depending on the toolset, https://github.com/jimon/premake-ninja/blob/master/ninja.lua#L401.

Which this is not extensible to other modules that define their own toolset (like the Emscripten module).

This allows external consumers of those toolsets to know what kind of
driver the tool uses.

This can be important in some cases, like in Ninja, which provides some
dependency handling features that need to change depending if the
compiler is GCC or MSVC-like.

The problem is that due to this information, the Ninja generator ends up
having hardcoded logic for this depending on the toolset,
https://github.com/jimon/premake-ninja/blob/master/ninja.lua#L401.

But this is not extensible to other modules that define their own
toolset (like the Emscripten module).
@tritao tritao marked this pull request as ready for review September 25, 2024 17:27
@samsinsane
Copy link
Member

Can you provide an example of how this is intended to be used? Based on what you have linked, it seems like the conditions would still exist but the Clang/GCC condition would reduce to a single check. That's not really a good enough improvement to me to justify this change.

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

Can you provide an example of how this is intended to be used? Based on what you have linked, it seems like the conditions would still exist but the Clang/GCC condition would reduce to a single check. That's not really a good enough improvement to me to justify this change.

	local driver
	if toolset.shared and toolset.shared.driver then
		driver = toolset.shared.driver
	end

	if toolset == p.tools.msc or driver == "msvc" then

To note, I left the original checks in the Ninja module even with the changes from this PR for backwards compatibility.

The main issue is that if third-party modules like the Emscripten extension want to define a new toolset (emcc in this case), then the Ninja module would have to be modified to know about how to handle it, even though its driver is GCC-compatible.

With this change, Emscripten can just do:

   emcc.shared = table.merge(clang.shared, {})

And the Ninja module will automatically know that its driver is GCC-compatible and be able to generate the correct code.

It really has nothing to do with cleaning up code but just allowing proper extensibility.

@samsinsane
Copy link
Member

It really has nothing to do with cleaning up code but just allowing proper extensibility.

You could achieve pretty much the same result by changing Ninja from if/elseif to if/else and all GCC-compatible toolsets would work without any modifications. Lots of things seem to be like GCC, but nothing else seems to be like MSVC. However, I think a better solution would be to actually allow for extensibility by allowing a toolset that isn't compatible with either to work without necessarily modifying/extending Ninja.

Having a really quick glance at the Ninja code, it seems like there's two differences between the MSVC and GCC versions. The command and deps, seems like deps is just msvc or gcc these seem like they're Ninja-specific(?) and should live in the Ninja module regardless. Which leaves command, both versions largely just boil down to wanting to specify the input file, the output file, the intermediate file, and the compile flag - these flags are duplicated in the gmake and gmake2 modules.

Perhaps starting with sharing the -c -o <output> -MF <intermediate> <input> pattern would give other toolsets a better path towards Ninja support. It would also allow make/ninja competitors to not have to duplicate everything again.

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

You could achieve pretty much the same result by changing Ninja from if/elseif to if/else and all GCC-compatible toolsets would work without any modifications.

Well that was one of my first solutions, but I don't think it's ideal, because it just assumes that everything that is not MSVC will be GCC-like, which may not be the case, for instance D or other languages like Zig. Or even if you provide a toolset that is based on extending msc from Premake, your suggestion will also not work (clang-cl custom toolset, for example).

This driver solution really just tells that a particular toolset driver is expected to work either like GCC or Clang, but you may want to not set it, in which case the Ninja module can report an error saying it cannot deal with such a toolset.

That is not just an imaginary possibility as some compilers are not GCC-driver compatible (for instance Haxe), but you still want to be able for it to be able to emit Make-style dep dependency output file data.

Lots of things seem to be like GCC, but nothing else seems to be like MSVC. However, I think a better solution would be to actually allow for extensibility by allowing a toolset that isn't compatible with either to work without necessarily modifying/extending Ninja.

So I've actually done that in the last few days, however unless your compilation model is C/C++ like, then the entire rules are not really applicable, just like in the gmake backend where you need custom code for C#. I have introduced some hooks to allow custom languages to implement their logic in the Ninja backend, but it doesn't make sense to re-use the ones from C/C++ model.

Having a really quick glance at the Ninja code, it seems like there's two differences between the MSVC and GCC versions. The command and deps, seems like deps is just msvc or gcc these seem like they're Ninja-specific(?) and should live in the Ninja module regardless.

In this specific PR, msvc or gcc just tell if a driver is compatible with MSVC or GCC, its not a Ninja-specific concept. It just so happens that Ninja uses the same term for the different concept of MSVC or GCC style dependency output file information.

Which leaves command, both versions largely just boil down to wanting to specify the input file, the output file, the intermediate file, and the compile flag - these flags are duplicated in the gmake and gmake2 modules.

Yes, I think some further factoring here should be done, but that is a different problem and seems unrelated to the issue at hand here.

As it stands, I still think the solution proposed in this PR is the best technical one, in that its quite simple (the bulk is just 3 lines)
and also more robust than the one you proposed.

@samsinsane
Copy link
Member

but that is a different problem and seems unrelated to the issue at hand here.

Then you might need to explain what the issue is because I'm not getting it from what you've said and linked.

To me, it seems like the issue is that the branch conditions are too strict, you want to loosen them and I'm saying just get rid of them. How is getting rid of them not the solution?

@tritao
Copy link
Contributor Author

tritao commented Sep 26, 2024

but that is a different problem and seems unrelated to the issue at hand here.

Then you might need to explain what the issue is because I'm not getting it from what you've said and linked.

To me, it seems like the issue is that the branch conditions are too strict, you want to loosen them and I'm saying just get rid of them. How is getting rid of them not the solution?

The main issue is if you have your own "derived" toolset, that extends one of Premake core toolsets, then external code cannot know if its based on GCC or MSVC-style driver. The solution you proposed, from what I understand, does:

if toolset == p.tools.msc then
else
-- assume gcc style here
end

But that assumption is not correct, as as I explained in my previous comment, has issues and can lead to broken assumptions. As far as I can tell, the only robust solution is for a toolset to define itself if its driver is based on MSVC or GCC-style flags.

@samsinsane
Copy link
Member

Okay, it looks like we're misunderstanding each other a bit here. The main solution I've been talking about is getting rid of both the if and the elseif, I don't think they're actually needed but I have also never used Ninja, so I don't know what I'm looking at.

Here's what I'm seeing:

if toolset == p.tools.msc then
  p.outln("CFLAGS=" .. all_cflags)
  p.outln("rule cc")
  p.outln("  command = " .. cc .. " $CFLAGS" .. " /nologo /showIncludes -c /Tc$in /Fo$out")
  p.outln("  description = cc $out")
  p.outln("  deps = msvc")
  p.outln("")
  -- More rules
elseif toolset == p.tools.clang or toolset == p.tools.gcc then
  -- pch stuff
  p.outln("CFLAGS=" .. all_cflags)
  p.outln("rule cc")
  p.outln("  command = " .. cc .. " $CFLAGS" .. force_include_pch .. " -x c -MF $out.d -c -o $out $in")
  p.outln("  description = cc $out")
  p.outln("  depfile = $out.d")
  p.outln("  deps = gcc")
  p.outln("")
  -- More rules
end

Why can't these be merged into a single path? Something like this?

-- pch stuff
p.outln("CFLAGS=" .. all_cflags)
p.outln("rule cc")
p.outln("  command = " .. cc .. " $CFLAGS" .. force_include_pch .. toolset.getccinoutflags(cfg, "$in", "$out.d", "$out"))
p.outln("  description = cc $out")
p.outln("  depfile = $out.d")
p.outln("  deps = " .. ninja.getdepsname(cfg, toolset)) -- This seems like it requires knowledge of Ninja? So, it shouldn't live in the toolset.
p.outln("")
-- More rules

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

Successfully merging this pull request may close these issues.

2 participants