Comments (6)
I think this is a great idea. It will be a cleaner separation of concerns, and will make users more cognizant of the potential side-effects when making CMake changes to their gems.
I also agree we should start by updating our templates first, and then do a sweep of updating our existing gems. This will likely need input from various gem owners to determine which things can/should be in their Public vs. Private APIs.
from sig-core.
I like the idea. Couple question:
-
How will the addition of a new target (potentially for every gem) cause an increase in configuration time, Visual Studio solution generation time and solution load time - I'm assuming compile time would not be affected much since they're just headers, but we could be looking at a significant number of new vs projects.
-
How will the additional target lead to added confusion for customers who want to do something common like adding a component with an ebus/interface and need to choose between the targets:
- gem
- gem.API
- gem.Editor
- gem.Editor.static
- gem.Static
- gem.Tests
Could we be increasing the difficulty for customers to learn how to add content to gems?
from sig-core.
Thanks for writing this RFC @lumberyard-employee-dm.
Overall I'm onboard with adding the new API targets to template gems. It paves the way for Gem authors to think on how they are going to present publicly their functionality while keeping the implementation private.
Feedback on the document
- It's a very nice intro. I miss in its second paragraph a mention of
${GemName}.Editor.Static
target, which is a very common static library our gems have as well. - The links to CppToolGem and DefaultGem seem to be broken.
- I'm missing in this RFC an example of a
CMakeList.txt
file of a common Gem with the normal targets plus the new API targets, with comments on each target explaining their scope and visibility. It'd help understanding better how it would look. - It'd also be great to show a Gem's
CMakeList.txt
file having a dependency on the API of another gem. - The first sentence in the disadvantages section is "The disadvantage is that existing gems will not new API targets exposed by new gems.", there is some error in the sentence around "will not new API targets" that I don't grasp what it means. Is this meant to say that existing gems won't be able to depend on this new API targets?
Question
- A gem is meant to be a compendium of libraries and we should not be limiting the user in what types of libraries they can add to it. Just to be clear, this proposal of adding API targets will be encouraged but optional, right? If the user still wants to create static libraries inside the Gem they can still do it, correct?
Concerns
-
Dependency and API readiness
When a target on gem A depends on a API target on gem B, that's not going to guarantee that the API of gem B is going to be ready before gem A calls it. The document doesn't explain how this runtime dependency works and what steps the users has to do to guarantee that an API is ready for when their dependents use it. -
Very high number of projects
The major concern would be the total number of projects we have with O3DE. Currently there are 526 projects on the VS solution. A normal Gem with runtime and editor functionality it's currently formed of 6 targets:
- {GemName}
- {GemName}.static
- {GemName}.Editor
- {GemName}.Editor.static
- {GemName}.Tests
- {GemName}.Editor.Tests
On top of this now we are going to add 2 more projects, making 8 targets per Gem. And this is ignoring the Benchmark target, which we should be encouraging to do as well.
- {GemName}
- {GemName}.API
- {GemName}.Private.Object
- {GemName}.Editor
- {GemName}.Editor.API
- {GemName}.Editor.Private.Object
- {GemName}.Tests
- {GemName}.Editor.Tests
This comes with the risk of potential disadvantages that the RFC does not mention. If O3DE scales in number of gems, the VS solution could become very large, slow, difficult to navigate, difficult to search and find things. I think at least the RFC should touch on this subject and mention a mitigation if there is any.
Alternative
There is an alternative that the RFC document doesn't mention, which is to keep using the static and module targets and educate the user to make a better usage of the visibility properties PUBLIC and PRIVATE on the Includes and Dependencies. The static target can have their includes PRIVATE, that way anybody outside the Gem that tries to depend on them won't find the headers. Then the module target will make the Include
directory PUBLIC and everything else PRIVATE (including build dependencies). There is an example of this in NvCloth
gem. This will avoid adding 2 new targets per Gem.
I still prefer the proposed solution of adding API targets, because it's more explicit. But if something goes wrong it's good that the RFC mentions this as an alternative.
from sig-core.
I like the idea. Couple question:
1. How will the addition of a new target (potentially for every gem) cause an increase in configuration time, Visual Studio solution generation time and solution load time - I'm assuming compile time would not be affected much since they're just headers, but we could be looking at a significant number of new vs projects. 2. How will the additional target lead to added confusion for customers who want to do something common like adding a component with an ebus/interface and need to choose between the targets: * gem * gem.API * gem.Editor * gem.Editor.static * gem.Static * gem.Tests
Could we be increasing the difficulty for customers to learn how to add content to gems?
For your first question:
The addition of new targets to Visual Studio shouldnot cause a large increase in solution loading time.
The addition of the .API .vcxproj projects is balanced against the fact the at the .Static
becoming a .Private.Object
target is smaller and does not contain the files from the Include directory.
Compilation time doesn't change at all.
Onto the second question:
The additional target will simplify users adding and EBus interface or AZ interface to their gem.
Those interfaces would go into the .API target if the interface is meant to be used by other gems, otherwise they should be placed into the .Private.Object
target.
Component themselves should only be placed in the .Private.Object
target as they shouldn't be directly available to gem users
from sig-core.
Thanks for writing this RFC @lumberyard-employee-dm.
Overall I'm onboard with adding the new API targets to template gems. It paves the way for Gem authors to think on how they are going to present publicly their functionality while keeping the implementation private.
Feedback on the document
* It's a very nice intro. I miss in its second paragraph a mention of `${GemName}.Editor.Static` target, which is a very common static library our gems have as well. * The links to CppToolGem and DefaultGem seem to be broken. * I'm missing in this RFC an example of a `CMakeList.txt` file of a common Gem with the normal targets plus the new API targets, with comments on each target explaining their scope and visibility. It'd help understanding better how it would look. * It'd also be great to show a Gem's `CMakeList.txt` file having a dependency on the API of another gem. * The first sentence in the disadvantages section is "_The disadvantage is that existing gems will not new API targets exposed by new gems._", there is some error in the sentence around "will not new API targets" that I don't grasp what it means. Is this meant to say that existing gems won't be able to depend on this new API targets?
Question
* A gem is meant to be a compendium of libraries and we should not be limiting the user in what types of libraries they can add to it. Just to be clear, this proposal of adding API targets will be encouraged but optional, right? If the user still wants to create static libraries inside the Gem they can still do it, correct?
Concerns
1. Dependency and API readiness When a target on gem A depends on a API target on gem B, that's not going to guarantee that the API of gem B is going to be ready before gem A calls it. The document doesn't explain how this runtime dependency works and what steps the users has to do to guarantee that an API is ready for when their dependents use it. 2. Very high number of projects The major concern would be the total number of projects we have with O3DE. Currently there are 526 projects on the VS solution. A normal Gem with runtime and editor functionality it's currently formed of 6 targets: * {GemName} * {GemName}.static * {GemName}.Editor * {GemName}.Editor.static * {GemName}.static.Tests * {GemName}.Editor.static.Tests
On top of this now we are going to add 2 more projects, making 8 targets per Gem. And this is ignoring the Benchmark target, which we should be encouraging to do as well.
* {GemName} * {GemName}.API * {GemName}.Private.Object * {GemName}.Editor * {GemName}.Editor.API * {GemName}.Editor.Private.Object * {GemName}.static.Tests * {GemName}.Editor.static.Tests
This comes with the risk of potential disadvantages that the RFC does not mention. If O3DE scales in number of gems, the VS solution could become very large, slow, difficult to navigate, difficult to search and find things. I think at least the RFC should touch on this subject and mention a mitigation if there is any.
Alternative
There is an alternative that the RFC document doesn't mention, which is to keep using the static and module targets and educate the user to make a better usage of the visibility properties PUBLIC and PRIVATE on the Includes and Dependencies. The static target can have their includes PRIVATE, that way anybody outside the Gem that tries to depend on them won't find the headers. Then the module target will make the
Include
directory PUBLIC and everything else PRIVATE (including build dependencies). There is an example of this inNvCloth
gem. This will avoid adding 2 new targets per Gem.I still prefer the proposed solution of adding API targets, because it's more explicit. But if something goes wrong it's good that the RFC mentions this as an alternative.
Answers: Feedback on the document
- I have fixed the links to the CppToolGem and the DefaultGem templates
- I updated the end of Suggested Design Description section to mention that the gem's Editor STATIC library target would also be converted an OBJECT library target.
- I have added an example of a TestGem added using the new public
.API
targets with the new.Private.Object
targets. - I have also clarified the disadvantages section to indicate that existing gems may not have the
.API
targets. This better emphasizes that the change is a "convention" to be followed and not an enforcement of specific targets in case the gem users are surprised by older gems not have the .API targets.
Answers: Question.
* A gem is meant to be a compendium of libraries and we should not be limiting the user in what types of libraries they can add to it. Just to be clear, this proposal of adding API targets will be encouraged but optional, right? If the user still wants to create static libraries inside the Gem they can still do it, correct?
To answer your question, the Gem Template is only meant has a starting place for users to create there own gems.
There is no CMake code in the O3DE build system to require users to use a specific target naming scheme or how many targets a gem can have.
Now what a gem is really a package with a descriptor "gem.json" file and a CMakeLists.txt script that allows hooking into the O3DE Build System as a plugin.
A gem doesn't even need to have any libraries at all. This is the case for an Asset only gem or a python script only gem which provides a list of files that the AssetProcessor or Editor can use without their being any built code.
Answers: Concerns
- Dependency and API readiness
That is out of scope for this document.
The Gem systems requires that gems can be loaded in any order.
Gems are initialized in several phases, where first the gems are all loaded and their component descriptors registered, followed by the required system components of all loaded gems being created.
Finally the activation of the System component uses a topological sort to order the gems based on their service dependencies.
That is covered in the Component Services doc.
Now for runtime dependencies that is actually explained in the Settings Registry Developer documentation on how gems are loaded. That documentation for the Settings Registry is not yet brought over.
- Very high number of projects
The users control how many targets there gem exposes. A user can create a gem using one of our Gem Templates and reduce it down to a single target or zero target or they can add 20 targets to a gem.
Also Visual Studio isn't the only IDE out there, that exposes CMake targets as projects(Xcode, VSCode CMake extension, CLion, etc...). For Visual Studio at least, the cost of loading a project and indexing is based on the content within the .vcxproj files.
In this case we aren't adding any new code files, we are splitting up a target into two smaller targets, which has near the same total load time.
Furthermore additional source files aren't being indexed, since the total number of files haven't increased, they are just in different .vcxprojs
The reason the Windows Visual Studio project has so many projects is because we have 80 gems within the engine.
We are actually trying to move gems out out of the repo which would reduce the number of Visual Studio Projects that are added to the o3de Engine solution.
The main mitigation for the high number of projects is to move to reduce the number of gems that come with the engine.
Using an external gem with an O3DE project only hooks that gem's CMakeLists.txt into the build system if that gem is used.
Answers: Alternative
For the alternative approach you mentioned, static library targets still have the issue that it duplicates the object files into a .lib
and .a
archive file which is results in gigabytes of disk space being used for non-SDK libraries.
Here it can be seen that we have 11GiB of STATIC libraries just for the non-monolithic profile configuration alone, most of which are private targets only used within the engine and not meant for consumption for non-engine gems.
So at the very least they should be transitioned over to OBJECT targets
Furthermore I think the naming scheme of ${GemName}.API
better conveys the intent that the target is meant for public consumption.
The ${GemName}.Static
target is not meant as a public external target for other gems to depend on.
Its purpose is to share code among the other targets internal to reduce the need to compile the same source file multiple times.
Primarily it is meant as a way for the ${GemName}
, ${GemName}.Editor
, ${GemName}.Test
and ${GemName}.Editor.Test
to re-use the object files that are built as part of the gem among them.
from sig-core.
Closing this out as the split has been performed
from sig-core.
Related Issues (20)
- Proposed SIG-Core meeting agenda for 1-APR-2022 - 0900 PT HOT 1
- Add a C++ Best Practice page to governance HOT 5
- RFC - ROS2 Gem HOT 5
- SIG Reviewer/Maintainer Nomination: @amzn-phist HOT 3
- Proposed SIG-Core meeting agenda for 2022-06-DD HOT 1
- Update SIG-Core README.md HOT 1
- Proposed SIG-Core meeting agenda for 3/AUG/2022 HOT 3
- Proposed RFC Feature Engine, Project and Gem Versions HOT 24
- SIG-Core meeting agenda for 14-SEPT-2022 HOT 1
- RFC: Add new log macros to replace AZTracePrintf HOT 9
- Create Roadmap View HOT 7
- SIG-Core Nomination/Election (2023-02-13 to 2023-03-03) HOT 8
- RFC: Improve Developer iteration workflow HOT 1
- Update Best-Practice Guide to include guidance on reducing compile time cost HOT 1
- Proposed RFC Feature New O3DE Archive System
- Proposed RFC Feature Quality/Scalability Framework HOT 10
- Proposed RFC Feature: No-Code Projects HOT 16
- RFC: Initial Project setup Asset Processor efficiency improvements HOT 2
- Proposed RFC Feature : Python Relocation and Virtual Environments HOT 6
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from sig-core.