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

Updates for cmake modules #1267

Merged
merged 9 commits into from
Sep 15, 2020
Merged

Updates for cmake modules #1267

merged 9 commits into from
Sep 15, 2020

Conversation

vhvb1989
Copy link
Member

  • Adding copyright to our in-house cmake modules
  • Adding reference to 3rd party cmake modules

Note: Notice and cgmanifest file to be updated in #1213

cmake-modules/AddTestCMocka.cmake Outdated Show resolved Hide resolved
find_package(cmocka CONFIG REQUIRED)

enable_testing()
include(CTest)

if (CMAKE_CROSSCOMPILING)
Copy link
Member

Choose a reason for hiding this comment

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

Was all this dead code?

Copy link
Member Author

Choose a reason for hiding this comment

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

not dead, but not really used by us.
This code is to use wine emulator on Windows for CYGIN or MINGW.
We don't have any documentation about how work with any of these (not even know if someone has tried).
Sp far, for Windows, we only use MSVC

cmake-modules/CheckAndIncludeCodeCov.cmake Show resolved Hide resolved
cmake-modules/CreateCodeCoverageTargets.cmake Outdated Show resolved Hide resolved
cmake-modules/gcc-arm-toolchain.cmake Outdated Show resolved Hide resolved
Comment on lines +1 to +2
# Copyright (c) Microsoft Corporation. All rights reserved.
# SPDX-License-Identifier: MIT
Copy link
Member

Choose a reason for hiding this comment

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

Nice catch. Any other files in the repo that might be missing this license header?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a cmake module in the form of a txt file inside eng/ folder.
This was created before using cmake modules. It can be updated later to be another cmake module.

Maybe yml files from eng/ should contain license as well

CMakeLists.txt Show resolved Hide resolved
Copy link
Member

@RickWinter RickWinter left a comment

Choose a reason for hiding this comment

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

Has CMocka been entered into the component governance site?

@@ -1,39 +1,15 @@
#
# Copyright (c) 2007 Daniel Gollub <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

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

Has CMocka been entered into the component governance site. Is this copyright usage approved?

Copy link
Member Author

Choose a reason for hiding this comment

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

will add the Notice and cgmanifest on #1213

This PR is just to update the licenses in cmake modules. Once this is merged, I will update the other PR to have all the Notices

@@ -124,6 +125,22 @@ if(NOT GCOV_PATH)
message(FATAL_ERROR "gcov not found! Aborting...")
endif() # NOT GCOV_PATH

####### C++ specific. Commenting from original or it fails for our gcc build for C99
Copy link
Member

Choose a reason for hiding this comment

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

Remove the commented code

Copy link
Member

@ahsonkhan ahsonkhan Sep 15, 2020

Choose a reason for hiding this comment

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

I recommended keeping it because it makes seeing the diff between our version and the source, easier, which helps validate and keep things in sync. Currently, we have no other difference other than this.

Up to you.

See the diff now: https://www.diffchecker.com/qrXQRqf5

Copy link
Member

Choose a reason for hiding this comment

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

Avoiding commented out code is ideal. Commented code tends to bit rot quickly.

As for the diff, Either way it shows up as diff. In one view all the lines are removed and in the other they are all commented.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed it

@vhvb1989 vhvb1989 requested a review from RickWinter September 15, 2020 18:25
Comment on lines +9 to +11
# Modifed version from https://github.com/xbmc/libssh/blob/667fb5f9a9c96f210583dbfb11755c43250c5e55/cmake/Modules/AddCMockaTest.cmake
#.rst:
# AddTestCMocka
Copy link
Member

Choose a reason for hiding this comment

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

Why did we rename the file originally? Keep it matching the source:

Rename the file:
AddTestCMocka.cmake -> AddCMockaTest.cmake

Copy link
Member Author

Choose a reason for hiding this comment

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

there's no difference. Can we keep this name so we don't make more changes to more files importing this module?
We are already mentioning this is a modified version and nothing stop us from using a different name. Isn't this a nit?

@vhvb1989 vhvb1989 requested a review from ahsonkhan September 15, 2020 18:57
Copy link
Member

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

LGTM

@vhvb1989 vhvb1989 merged commit 8b35c75 into Azure:master Sep 15, 2020
@vhvb1989 vhvb1989 deleted the sync-code-cov-cmake-module branch September 15, 2020 19:41
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.

3 participants