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

Jump to bottom

Use Reuse tool for license handling #1460

Merged
merged 4 commits into from
Nov 24, 2023
Merged

Use Reuse tool for license handling #1460

merged 4 commits into from
Nov 24, 2023

Conversation

MarcelKoch
Copy link
Member

This PR enables using the reuse tool [1] for adding our license to our files. This replaces the current add_license.sh script. The tool is automatically invoked as part of our pre-commit hooks.

Sidenote: There is an official pre-commit hook for the reuse tool, but only for the linting. This has several drawbacks, 1. it requires that EVERY file has a license (I think we should still do that, but in another PR), 2. our current workflow is to add the license automatically, not just warn about it. There is some discussion for adding an official hook to do the annotation, so perhaps we can remove our manual hook in the future.

@MarcelKoch MarcelKoch self-assigned this Nov 14, 2023
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system. labels Nov 14, 2023
@MarcelKoch MarcelKoch mentioned this pull request Nov 14, 2023
Open
@MarcelKoch MarcelKoch mentioned this pull request Nov 14, 2023
Merged
1 task
yhmtsai
yhmtsai reviewed Nov 15, 2023
View reviewed changes
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@MarcelKoch MarcelKoch force-pushed the use-fixed-clang-format branch 2 times, most recently from e2418ca to 1cce135 Compare November 16, 2023 14:53
Base automatically changed from use-fixed-clang-format to develop November 16, 2023 18:02
@yhmtsai
Copy link
Member

yhmtsai commented Nov 20, 2023

something is not rebased yet

@MarcelKoch MarcelKoch changed the base branch from develop to pre-commit-update_ginkgo_header November 20, 2023 08:09
@MarcelKoch MarcelKoch force-pushed the pre-commit-update_ginkgo_header branch from 370e9cc to e43ec94 Compare November 20, 2023 13:56
@greole
Copy link
Collaborator

greole commented Nov 20, 2023

Sidenote: There is an official pre-commit hook for the reuse tool, but only for the linting. This has several drawbacks, 1. it requires that EVERY file has a license (I think we should still do that, but in another PR)
Cant this be avoided by a .reuse/dep5 file, which would some entries like

Format: https://www.debian.org/doc/packaging-manuals/copyright-format/1.0/    
Upstream-Name: NeoFOAM                                                       
Upstream-Contact: NeoFOAM authors    
Source: https://git.example.com/jane/my-project
                                                                                                               
Files: .github/*                                                                
Copyright: 2023 NeoFOAM authors                                                 
License: MPL-2.0

@MarcelKoch
Copy link
Member Author

@greole yes, but the major drawback is that the official hook doesn't automatically add the license.

yhmtsai
yhmtsai reviewed Nov 21, 2023
View reviewed changes
.github/check-format.sh Outdated Show resolved Hide resolved
CMakeLists.txt
Comment on lines -380 to -384

add_custom_target(add_license
COMMAND ${Ginkgo_SOURCE_DIR}/dev_tools/scripts/add_license.sh
WORKING_DIRECTORY ${Ginkgo_SOURCE_DIR})
add_dependencies(format add_license)
Copy link
Member

Choose a reason for hiding this comment

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

the format will handle the license now, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

the pre-commit hook will handle it.

@MarcelKoch MarcelKoch changed the base branch from pre-commit-update_ginkgo_header to develop November 24, 2023 09:43
pratikvn
pratikvn previously approved these changes Nov 24, 2023
View reviewed changes
Copy link
Member

@pratikvn pratikvn left a comment

Choose a reason for hiding this comment

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

LGTM!

include/ginkgo/core/log/papi.hpp
@@ -12,11 +12,11 @@
#if GKO_HAVE_PAPI_SDE


#include <sde_lib.h>
Copy link
Member

Choose a reason for hiding this comment

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

Was this automatic ? Should this probably be in a separate section ?

.github/check-format.sh Show resolved Hide resolved
yhmtsai
yhmtsai previously approved these changes Nov 24, 2023
View reviewed changes
include/ginkgo/core/log/papi.hpp
Comment on lines +15 to -19
#include <sde_lib.h>
#include <cstddef>
#include <iostream>
#include <map>
#include <mutex>
#include <sde_lib.h>
Copy link
Member

Choose a reason for hiding this comment

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

should it be changed?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is missing format_header. But I think we should just leave it, since it will most likely change again in #1466.

@MarcelKoch MarcelKoch dismissed stale reviews from pratikvn and yhmtsai November 24, 2023 13:59

stale

@MarcelKoch MarcelKoch merged commit 0775637 into develop Nov 24, 2023
8 of 15 checks passed
@MarcelKoch MarcelKoch deleted the reuse-script branch November 24, 2023 17:48
@sonarcloud SonarCloud
Copy link

sonarcloud bot commented Nov 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

warning The version of Java (11.0.3) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pratikvn pratikvn pratikvn approved these changes

@yhmtsai yhmtsai yhmtsai approved these changes

Assignees

@MarcelKoch MarcelKoch

Labels
reg:build This is related to the build system. reg:ci-cd This is related to the continuous integration system.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MarcelKoch @yhmtsai @greole @pratikvn @ginkgo-bot