Skip to content
This repository has been archived by the owner on Jul 19, 2018. It is now read-only.

layers:Remove includes of vk_safe_struct.cpp #1866

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

tobine
Copy link
Contributor

@tobine tobine commented Jun 12, 2017

After looking at these includes of vk_safe_struct.cpp I believe it was
just a hack that worked and has persisted in the code. This change
pulls the source into the layers that depend on it at build time and is
a better solution, in my opinion.

The one quirk is that the we now generate vk_struct.cpp in the base dir
and in the layers dir. Since the file is required by some layer shared
libs I had to generate in the layers dir, but there are also some
dependencies on the file in the top-level dir that I didn't care to
investigate further so I just genrate it in two places.
I'm sure this could be cleaned up if someone with more cmake knowledge
and/or patience than me cares to look into it further.

Copy link
Collaborator

@chrisforbes chrisforbes left a comment

Choose a reason for hiding this comment

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

This looks like it's getting there (finally clean on travis!).

@@ -38,6 +38,7 @@ LOCAL_SRC_FILES += $(SRC_DIR)/layers/descriptor_sets.cpp
LOCAL_SRC_FILES += $(SRC_DIR)/layers/buffer_validation.cpp
LOCAL_SRC_FILES += $(SRC_DIR)/layers/shader_validation.cpp
LOCAL_SRC_FILES += $(SRC_DIR)/layers/vk_layer_table.cpp
LOCAL_SRC_FILES += $(LAYER_DIR)/include/vk_safe_struct.cpp
Copy link
Collaborator

Choose a reason for hiding this comment

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

Don't much like .cpp files in include/

@tobine tobine force-pushed the tobin_fix_safe_struct_gen branch from 46cf072 to e67f6c8 Compare June 13, 2017 14:31
@tobine
Copy link
Contributor Author

tobine commented Jun 13, 2017

I updated android codegen to put vk_safe_struct.cpp into "common" dir instead of "include" and use it from there in layer libs.

@tobine tobine force-pushed the tobin_fix_safe_struct_gen branch 4 times, most recently from 0e0b433 to 96b139f Compare June 13, 2017 15:43
Copy link
Contributor

@stroyan stroyan left a comment

Choose a reason for hiding this comment

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

Unfortunately, the weird use of include for vk_safe_struct.cpp is still required to prevent visual studio from doing conflict file generation during parallel builds.
Perhaps the file should be changed to a generated .h header to match the way it is used.

@chrisforbes
Copy link
Collaborator

@stroyan or we can move this down a level into a static lib, and have the layers depend on that?

@mark-lunarg
Copy link
Collaborator

@stroyan, any reply to @chrisforbes?

@mark-lunarg
Copy link
Collaborator

@tobine, @stroyan, any update here, or can we just get by with things the way they are now?

@stroyan
Copy link
Contributor

stroyan commented Dec 15, 2017

It would be nice to get back to using a proper add_library rule for using vk_safe_struct.cpp.
Looking at the current https://cmake.org/Wiki/CMake_FAQ description of this problem I see
that there is an alternative workaround of making one of the layers depend on the other.
Then the two layers that use vk_safe_struct.cpp will not be built in parallel.
Then visual studio will never clobber vk_safe_struct.cpp.
That could be accomplished by adding one more rule to layers/CMakeLists.txt in the section
starting with- # Core validation has additional dependencies
Add-

# Dependency on unique_objects is only to prevent parallel build of both layers.
# Visual Studio has trouble with clobbering vk_safe_struct.cpp if both build in parallel.
add_dependencies(VkLayer_core_validation VkLayer_unique_objects)

I don't have a system available right now to confirm if visual studio does the right thing with that additional rule.

@tobine
Copy link
Contributor Author

tobine commented Dec 15, 2017

Updated with WA suggested by @stroyan and re-testing.

After looking at these includes of vk_safe_struct.cpp I believe it was
just a hack that worked and has persisted in the code. This change
pulls the source into the layers that depend on it at build time and is
a better solution, in my opinion.

The one quirk is that the we now generate vk_struct.cpp in the base dir
and in the layers dir. Since the file is required by some layer shared
libs I had to generate in the layers dir, but there are also some
dependencies on the file in the top-level dir that I didn't care to
investigate further so I just genrate it in two places.
I'm sure this could be cleaned up if someone with more cmake knowledge
and/or patience than me cares to look into it further.

For the android build updated the codegen to put vk_safe_struct.cpp in
common dir so it could be added to libs from there.
Core validation and unique_objects layers both use generated
safe_struct.cpp file. To prevent a potential build race condition
on the windows build, add a dependency between those two layers.
@mark-lunarg
Copy link
Collaborator

@tobine, this repository will close for write access on Sunday, 5/13/2018. If it is pushed before that time it will be present in the follow-on Vulkan-ValidationLayers repository on Monday, otherwise a new PR will be required in the new repo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants