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

DALI as a git submodule #1924

Merged
merged 2 commits into from
May 4, 2020
Merged

DALI as a git submodule #1924

merged 2 commits into from
May 4, 2020

Conversation

AlexBula
Copy link
Contributor

Why we need this PR?

Pick one, remove the rest

  • It fixes a bug: It allows to add DALI to an another project as a git submodule and link against it

What happened in this PR?

Fill relevant points, put NA otherwise. Replace anything inside []

  • What solution was applied:
    Change CMakeLists paths to ${PROJECT_SOURCE_DIR}, add export of DALI
  • Affected modules and functionalities:
    Few path in the CMakeLists were changed because they assumed DALI was the main project that was currently built
  • Key points relevant for the review:
    [ Describe here what is the most important part that reviewers should focus on. ]
  • Validation and testing:
    Checked if DALI is building after changes
  • Documentation (including examples):
    [ Describe here if documentation and examples were updated. ]

JIRA TASK: NA

@AlexBula AlexBula changed the title Changes in the CMakeLists in order to build dali as a submodule DALI as a git submodule Apr 30, 2020
CMakeLists.txt Outdated
Comment on lines 200 to 201

export(PACKAGE dali)
Copy link
Member

Choose a reason for hiding this comment

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

I'd approach it a little bit differently: I think we should export individual targets within some namespace, e.g. using this:

export(TARGETS [target1 [target2 [...]]] [NAMESPACE <namespace>]
       [APPEND] FILE <filename> [EXPORT_LINK_INTERFACE_LIBRARIES])

There you could export dali, dali_operators, dali_core etc....

Usage could be following:

add_subdirectory(extern/DALI)
target_link_libraries(mylib DALI::dali_core DALI::dali_kernels)

On top of that, we can expose convenient target that gathers every DALI target, so that user can go:

add_subdirectory(extern/DALI)
target_link_libraries(mylib DALI::DALI)

But I'd like to hear what others think about it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are probably right :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed so only the listed targets are exported

@AlexBula
Copy link
Contributor Author

!build

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1291181]: BUILD STARTED

@dali-automaton
Copy link
Collaborator

CI MESSAGE: [1291181]: BUILD PASSED

Comment on lines +201 to +204
add_library(DALI::dali ALIAS dali)
add_library(DALI::dali_core ALIAS dali_core)
add_library(DALI::dali_kernels ALIAS dali_kernels)
add_library(DALI::dali_operators ALIAS dali_operators)
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to additionally create one target, that puts together all these four?

One to rule them all ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not think so, at least I did not find one

@AlexBula AlexBula merged commit c865eda into NVIDIA:master May 4, 2020
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.

5 participants