Add frameworkdirs support to gmake and gmake2 with gcc/clang toolsets… #1661

noresources · 2021-06-30T14:17:41Z

… on macOS systems

Add new optional parameter to toolset.getincludedirs(dirs, sysdirs, frameworkdirs)
Translate frameworkdirs to -F build & linker flags
Add tests

Anything else we should know?
It should be easy to update CodeLite too but I can't really test it.

Did you check all the boxes?

Focus on a single fix or feature; remove any unrelated formatting or code changes
Add unit tests showing fix or feature works; all tests pass
Mention any related issues (put closes #XXXX in comment to auto-close issue when PR is merged)
Follow our coding conventions
Minimize the number of commits
Align documentation to your changes

samsinsane · 2021-07-01T15:09:30Z

src/tools/gcc.lua

 			dir = project.getrelative(cfg.project, dir)
 			table.insert(result, '-I' .. p.quoted(dir))
 		end
+
+		if (table.contains(os.getSystemTags(cfg.system), "darwin")
+			and cfg.frameworkdirs ~= nil and #cfg.frameworkdirs) then


This should be on one line. This isn't using the right variable, shouldn't be using cfg. for frameworkdirs. Also, this is not consistent with the other dirs blocks by checking the value of cfg.frameworkdirs/frameworkdirs.

Ok for the "one-line".

The use of cfg. is a copy-paste-is-evil mistake (from gcc.getLibraryDirectories(cfg)), that I previously fixed and re-introduced while rebasing for this PR...

The test on frameworkdirs value is made to minimize changes in code. Otherwise it will
require to add , {} on all toolset.getincludedirs() calls. I suggest to keep it for now.

The test on frameworkdirs value is made to minimize changes in code. Otherwise it will
require to add , {} on all toolset.getincludedirs() calls. I suggest to keep it for now.

Sorry to be pedantic, but isn't that why you have the or {} inside? Which is how the sysdirs loop works too. Checking if frameworkdirs is not nil isn't necessary because you're already doing ipairs(frameworkdirs or {})?

src/tools/gcc.lua

Jarod42 · 2021-07-09T07:37:46Z

modules/gmake/gmake_cpp.lua

@@ -521,7 +521,7 @@ end


 	function make.includes(cfg, toolset)
-		local includes = toolset.getincludedirs(cfg, cfg.includedirs, cfg.sysincludedirs)
+		local includes = toolset.getincludedirs(cfg, cfg.includedirs, cfg.sysincludedirs, cfg.frameworkdirs)


Should probably be done for all exporters.
Seems you miss several getincludedirs in project (even if I'm not sure if resource need it).

This "miss" was intentional. I don't think resflags needs framework dirs.

For resources, it might be OK, but I would pass nil explicitly.

For gmake2, only for "per-file" configuration has been done.
Missing for Codelite generator...

I am not against an explicit nil, however there is currently no toolset.getincludedirs(cfg, inc, nil) when sysdir is not mandatory.
About gmake2, Ok, I didn't understand your first message quite well. This will be fixed.
About CodeLite, I said in the PR message I am not using CodeLite, have no knowledge about it and I can't test it.

sysdir is missing only for resources flags (I wonder it is not a bug BTW).

You should add frameworkdirs for all non-resources flags call (for all generators).

It is the toolset which "manages" it. (as msvc which does nothing).
if generator doesn't use directly compiler flags, it iterates directly on cfg.includedirs or similar, and then indeed, it would require knowledge of the generator.

For test, I started https://github.com/Jarod42/premake-sample-projects to test with projects with multiple generators.
Nothing platform specific yet, as frameworks

If you can provide sample test project (usable in github action) (a variant of project-01 which test includedirs), I can add it.

@Jarod42 It is okay if contributors only want to target a single toolset. Not everyone has access to every toolset; allowing people to contribute where they can is more important. And please do not confuse things by asking people to provide you with tests for a separate repository; unit tests are all that we require at this time. Thanks!

@starkos: What bothers me is that, then projects are generator-specific without any errors/warnings for users when used with incompatible generators.

Sorry, I didn't want to confuse people with extra tests in another repository.

Notice that you have tests just for gcc, but not for gmake/gmake2.

What bothers me is that, then projects are generator-specific without any errors/warnings for users when used with incompatible generators.

The alternative is that people don't contribute. We've been down that path before. I do agree that we need to do a better job of surfacing compatibility information, and the documentation seems an appropriate place to do that.

@Jarod42 If you have general comments on broad topics ("features should be implemented for all generators") please use the provided Discussions rather than cluttering up the PR review notes. I have no longer have any idea what change was requested by this thread, and if it has been done or not.

modules/gmake2/gmake2_cpp.lua

Jarod42 · 2021-07-09T07:40:02Z

src/tools/gcc.lua

 		local result = {}
 		for _, dir in ipairs(dirs) do
 			dir = project.getrelative(cfg.project, dir)
 			table.insert(result, '-I' .. p.quoted(dir))
 		end
+
+		if (table.contains(os.getSystemTags(cfg.system), "darwin") and frameworkdirs ~= nil) then
+			for _, dir in ipairs(frameworkdirs or {}) do


Below doesn't have or {}.

and frameworkdirs ~= nil is probably the one to remove here

… on macOS systems * Add new optional parameter to toolset.getincludedirs(dirs, sysdirs, frameworkdirs) * Translate frameworkdirs to -F<path> build & linker flags * Add tests Co-authored-by: Joris Dauphin <Jarod42@users.noreply.github.com> Co-authored-by: Samuel Surtees <samsinsane@users.noreply.github.com>

noresources · 2021-09-08T09:58:14Z

Hello,
I finally had some spare time and motivation to run a macOS VM, install CodeLite and test frameworkdirs support.
I also add the runpathdirs support to it (one line change).
Finally I rebased/cleanup commits on top of master.
Hope this will allow to close this PR.

starkos

Approving, but will let @samsinsane make the call on the merge since they've been more involved in the review.

starkos · 2021-09-14T12:50:20Z

Oh shoot, I see @samsinsane already approved…merging…

samsinsane requested changes Jul 1, 2021

View reviewed changes

noresources force-pushed the toolset-frameworkdirs branch 2 times, most recently from 16698cd to 53e3930 Compare July 2, 2021 12:18

samsinsane approved these changes Jul 6, 2021

View reviewed changes

Jarod42 suggested changes Jul 9, 2021

View reviewed changes

sheepsteak mentioned this pull request Aug 3, 2021

Use frameworks in Premake generator conan-io/conan#9371

Merged

noresources force-pushed the toolset-frameworkdirs branch from cfcb119 to 1fdd175 Compare September 8, 2021 08:26

Renaud Guillard and others added 2 commits September 8, 2021 11:08

frameworkdirs and runpathdirs support for CodeLite generator

f78741f

noresources force-pushed the toolset-frameworkdirs branch from 1fdd175 to f78741f Compare September 8, 2021 09:08

Merge branch 'master' into toolset-frameworkdirs

0c87f0c

starkos approved these changes Sep 14, 2021

View reviewed changes

starkos merged commit 0dd9c4b into premake:master Sep 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add frameworkdirs support to gmake and gmake2 with gcc/clang toolsets… #1661

Add frameworkdirs support to gmake and gmake2 with gcc/clang toolsets… #1661

noresources commented Jun 30, 2021

samsinsane Jul 1, 2021

noresources Jul 2, 2021

samsinsane Jul 6, 2021

Jarod42 Jul 9, 2021 •

edited

Loading

noresources Jul 9, 2021

Jarod42 Jul 12, 2021

noresources Jul 12, 2021

Jarod42 Jul 13, 2021

starkos Sep 14, 2021

Jarod42 Sep 15, 2021

starkos Sep 15, 2021

Jarod42 Jul 9, 2021

noresources Jul 9, 2021

noresources commented Sep 8, 2021

starkos left a comment

starkos commented Sep 14, 2021

Add frameworkdirs support to gmake and gmake2 with gcc/clang toolsets… #1661

Add frameworkdirs support to gmake and gmake2 with gcc/clang toolsets… #1661

Conversation

noresources commented Jun 30, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jarod42 Jul 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noresources commented Sep 8, 2021

starkos left a comment

Choose a reason for hiding this comment

starkos commented Sep 14, 2021

Jarod42 Jul 9, 2021 •

edited

Loading