-
Notifications
You must be signed in to change notification settings - Fork 115
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
Simple CMake in AzureDevOps #3
Conversation
sdk/keyvault/keyvault/CMakeLists.txt
Outdated
@@ -0,0 +1,3 @@ | |||
cmake_minimum_required (VERSION 3.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This minimum version doesn't match what's in the template https://github.com/Azure/azure-sdk-for-c/pull/2/files (3.15)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I started it before. I will update to the latest template as soon as it merged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be at least 3.12, no reason to revert to a lower version here after setting 3.12 in the parent project
pool: | ||
vmImage: $(vm.image) | ||
steps: | ||
- task: CMake@1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bsiegel currently, it tries to build all projects by using CMakeLists.txt
in the root. I will need to update it to $(ServiceDirectory)
.
@jhendrixMSFT, @barcharcraz please, review :-) |
@@ -0,0 +1,3 @@ | |||
cmake_minimum_required (VERSION 3.12) | |||
project(az) | |||
add_subdirectory(sdk/storage/storage) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is /storage repeated, or is this just temporary to give the build system something to build and will be removed later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm following a pattern from other SDKs. AFAIK, some services may have several libraries/projects etc.
build.ps1
Outdated
$location = $PSScriptRoot | ||
$build = Join-Path $location "build" | ||
CMake -S $location -B $build | ||
CMake --build $build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing new-line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also does this mean we're standardizing on PS for cross-plat build scripting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhendrixMSFT, @bsiegel as far as I know, PS core is the only one that is installed by default on all Azure Dev Ops hosts (Windows, Linux, Mac OS X). This specific build.ps1
is not used by CI and it's for developers. The problem is that by default CMake outputs everything into the current directory :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's better to just not add these kinds of build scripts, but that's just me.
DPSv2 Codec changes
No description provided.