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.

Sign up for GitHub

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

fix build with samples on linux #3724

Closed
wants to merge 4 commits into from
Closed

fix build with samples on linux #3724

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3042e78
fix build with samples on linux
vhvb1989 Jun 8, 2022
435beb8
use shared code for service instead of an interface library
vhvb1989 Jun 8, 2022
3e78ddb
Merge branch 'main' of github.com:Azure/azure-sdk-for-cpp into fix-bu…
vhvb1989 Jun 8, 2022
c4cdde6
remove msg
vhvb1989 Jun 8, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,6 @@ include(AzureVersion)

if(BUILD_SAMPLES)
add_subdirectory(samples/helpers/get-env)
add_subdirectory(samples/helpers/service)
Copy link
Member

Choose a reason for hiding this comment

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

Let's try to find the other way... Basically, what we have is header-only library, let's fix cmake so that it works on linux too. I would really like to avoid the ..\..\.., and make it look like any other azure-sdk library. Even at a cost of it having a .cpp file.

Copy link
Member

@antkmsft antkmsft Jun 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Given that service is supposed to be a placeholder for azure-anyservice, I thikn it is better to add a .cpp file. We can make it an OBJECT library similar to get-env, consuming it is semantically the same as regular library, unlike INTERFACE library (http://www.mariobadr.com/creating-a-header-only-library-with-cmake.html#:~:text=Creating%20a%20Header-Only%20CMake%20Target%20Creating%20an%20interface,is%20not%20meant%20to%20generate%20any%20build%20output.).

Copy link
Member

Choose a reason for hiding this comment

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

@vhvb1989, how about this? - #3728

endif()

# sub-projects
Expand Down
17 changes: 0 additions & 17 deletions samples/helpers/service/CMakeLists.txt
View file
Open in desktop

This file was deleted.

21 changes: 0 additions & 21 deletions samples/helpers/service/LICENSE
View file
Open in desktop

This file was deleted.

13 changes: 13 additions & 0 deletions samples/helpers/service/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
# Generic Service for Samples

This is a helper library for samples that provides a generic service client library.

## How to use

Include the header from the CMake project using `target_include_directories`. For example, use

```cmake
# NOTE: Use shared-code only within .cpp files. DO NEVER consume the shared-code from header files.
target_include_directories(
cmake-target-name
PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../path/to/samples/inc>
)
```
30 changes: 25 additions & 5 deletions sdk/identity/azure-identity/samples/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,26 +8,46 @@ set(CMAKE_CXX_STANDARD 14)
set(CMAKE_CXX_STANDARD_REQUIRED True)

add_executable(chained_token_credential_sample chained_token_credential.cpp)
target_link_libraries(chained_token_credential_sample PRIVATE azure-identity service)
target_link_libraries(chained_token_credential_sample PRIVATE azure-identity)
target_include_directories(chained_token_credential_sample PRIVATE .)
target_include_directories(
chained_token_credential_sample PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../../samples/helpers/service/inc>
)
create_per_service_target_build_for_sample(identity chained_token_credential_sample)

add_executable(client_certificate_credential_sample client_certificate_credential.cpp)
target_link_libraries(client_certificate_credential_sample PRIVATE azure-identity service get-env-helper)
target_link_libraries(client_certificate_credential_sample PRIVATE azure-identity get-env-helper)
target_include_directories(client_certificate_credential_sample PRIVATE .)
target_include_directories(
client_certificate_credential_sample PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../../samples/helpers/service/inc>
)
create_per_service_target_build_for_sample(identity client_certificate_credential_sample)

add_executable(client_secret_credential_sample client_secret_credential.cpp)
target_link_libraries(client_secret_credential_sample PRIVATE azure-identity service get-env-helper)
target_link_libraries(client_secret_credential_sample PRIVATE azure-identity get-env-helper)
target_include_directories(client_secret_credential_sample PRIVATE .)
target_include_directories(
client_secret_credential_sample PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../../samples/helpers/service/inc>
Copy link
Member

Choose a reason for hiding this comment

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

Instead of navigating ../../../.. etc, why not put this in a variable once and then use the variable everywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

We decided not to do that for CMake files long time ago, as per Charlie's advice (since we were doing the C SDK)

I remember because I used to set all re-usable strings from a CMake file in the top and then I would just use the variables everywhere.

So, yeah, I am not against it, but I have been following that advise of keeping the duplication.
The only real benefit of the variables was to make it easier to update in the future, but using any editor from this days can select and replace all occurrences at the same time (and I couldn't argue with that )

Copy link
Contributor

Choose a reason for hiding this comment

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

@barcharcraz can you please share some guidance and rationale for when and why we should avoid creating reusable string variables within CMake files?

)
create_per_service_target_build_for_sample(identity client_secret_credential_sample)

add_executable(environment_credential_sample environment_credential.cpp)
target_link_libraries(environment_credential_sample PRIVATE azure-identity service)
target_link_libraries(environment_credential_sample PRIVATE azure-identity)
target_include_directories(environment_credential_sample PRIVATE .)
target_include_directories(
environment_credential_sample PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../../samples/helpers/service/inc>
)
create_per_service_target_build_for_sample(identity environment_credential_sample)

add_executable(managed_identity_credential_sample managed_identity_credential.cpp)
target_link_libraries(managed_identity_credential_sample PRIVATE azure-identity service)
target_link_libraries(managed_identity_credential_sample PRIVATE azure-identity)
target_include_directories(managed_identity_credential_sample PRIVATE .)
target_include_directories(
managed_identity_credential_sample PRIVATE
$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/../../../../samples/helpers/service/inc>
)
create_per_service_target_build_for_sample(identity managed_identity_credential_sample)