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

Reduce license header #1404

Merged
merged 10 commits into from
Nov 14, 2023
Merged

Reduce license header #1404

merged 10 commits into from
Nov 14, 2023

Conversation

MarcelKoch
Copy link
Member

@MarcelKoch MarcelKoch commented Sep 11, 2023

This PR changes how we handle our license declaration. It moves from the current block of license (~30 lines) to 3/4 lines as defined by https://reuse.software/. The new license header will now read as:

// SPDX-FileCopyrightText: 2017-2023 The Ginkgo authors
//
// SPDX-License-Identifier: BSD-3-Clause

It also updates the add_license.sh script accordingly and changes the top-level license file. The latter might lead to issues with githubs automatic license detection.

Todo:

  • check github's license detection - will just use the current license file

@MarcelKoch MarcelKoch self-assigned this Sep 11, 2023
@ginkgo-bot ginkgo-bot added reg:build This is related to the build system. reg:testing This is related to testing. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:benchmarking This is related to benchmarking. type:solver This is related to the solvers type:preconditioner This is related to the preconditioners type:matrix-format This is related to the Matrix formats type:factorization This is related to the Factorizations type:reordering This is related to the matrix(LinOp) reordering reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. type:multigrid This is related to multigrid type:stopping-criteria This is related to the stopping criteria mod:all This touches all Ginkgo modules. labels Sep 11, 2023
GINKGO_ROOT_DIR="."


LICENSE_FILE="${GINKGO_ROOT_DIR}/LICENSE"
GINKGO_LICENSE_BEACON="******************************<GINKGO LICENSE>******************************"
Copy link
Member

Choose a reason for hiding this comment

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

does the structure allow the beacon?
I am wondering about some scripts that rely on the BEACON detection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I didn't realize that other scripts also use the beacon. I will adjust them.

@MarcelKoch MarcelKoch added the 1:ST:ready-for-review This PR is ready for review label Sep 15, 2023
@upsj upsj self-requested a review September 18, 2023 12:39
@greole
Copy link
Collaborator

greole commented Oct 23, 2023

REUSE provides a python tool to verify reuse compliance. We could also add this to our CI pipeline. However, this will additionally require to:

  • add some license information to our cmake files.
  • I think reuse allows to specify a .license file for a folder of binary data, like our assets folder

@MarcelKoch
Copy link
Member Author

@greole I'm very much in favor of that. Here is the old pr for that: #1390. This PR is a first step towards that.

Copy link
Collaborator

@greole greole left a comment

Choose a reason for hiding this comment

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

LGTM

@MarcelKoch
Copy link
Member Author

Note: Since the format CI is based on the jobs defined in develop, that job will always fail. I will mirror this PR in my fork to check if the CI behaves as expected.

@MarcelKoch
Copy link
Member Author

See MarcelKoch#10 for CI check. The new license header is used properly in the CI. It was necessary to change the develop branch in my fork to already enable the changes to check-format as they are in this PR.

@yhmtsai
Copy link
Member

yhmtsai commented Nov 13, 2023

I saw you change some script to adapt the changes of beacon
Does SPDX allow the following? With the following, we do not need to change many scripts

/*******************************<GINKGO LICENSE>******************************
* SPDX-FileCopyrightText: 2017-2023 The Ginkgo authors
*
* SPDX-License-Identifier: BSD-3-Clause
******************************<GINKGO LICENSE>*******************************/

With // approach, it require all script recognize the SPDX fully. Previously, it is only handled through add_license.sh and others only detect or add beacon.

@MarcelKoch
Copy link
Member Author

I would like to keep the new license as short as possible, that's why I'm using //. Also, I think don't think the beacon approach is supported by the reuse tool, to which I will move to in the next step.

Copy link
Member

@upsj upsj left a comment

Choose a reason for hiding this comment

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

LGTM! As the license is always the first part of the file, and much more structured, I also tend to agree we no longer need the beacon

@yhmtsai
Copy link
Member

yhmtsai commented Nov 13, 2023

I do not fully agree with it's more structured. beacon way does not hurt the structure but gives easy way to detect the block. something like https://github.com/fsfe/reuse-example/blob/main/src/main.c

@MarcelKoch
Copy link
Member Author

I meant that the reuse tool doesn't support it for adding new license header. Also, I don't see why we need to detect the block. Adding the license will be handled by reuse, which can detect it correctly, and the formatting by clang.

Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I consider it maybe from another side.
We need to pay additional attension on changing beacon to //. handling license in two or three places may not be good for reuse/consistense.
I originally think it only need to change or create another file (SPDX...)for add_licences/format_header. Other scripts work smoothly.
beacon way is widely used by tranditional license in my mind, so I am also suprised reuse tool may not support it.
request changes mainly for the wrong script and the inconsistence between "the" and "The"

dev_tools/oneapi/convert_source.sh Outdated Show resolved Hide resolved
dev_tools/scripts/add_license.sh Show resolved Hide resolved
Copy link

sonarcloud bot commented Nov 14, 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

0.0% 0.0% Coverage
0.0% 0.0% Duplication

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

- fix script
- always use `The` in copyright

Co-authored-by: Yu-Hsiang M. Tsai <yhmtsai@gmail.com>
Copy link
Member

@yhmtsai yhmtsai left a comment

Choose a reason for hiding this comment

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

I do not hold the concern for beacon now.

cmake/Modules/CudaArchitectureSelector.cmake Outdated Show resolved Hide resolved
@ginkgo-bot

This comment was marked as off-topic.

@MarcelKoch MarcelKoch merged commit fd61672 into develop Nov 14, 2023
11 of 15 checks passed
@MarcelKoch MarcelKoch deleted the reuse-license-minimal branch November 14, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1:ST:ready-for-review This PR is ready for review 1:ST:skip-full-test mod:all This touches all Ginkgo modules. reg:benchmarking This is related to benchmarking. reg:build This is related to the build system. reg:documentation This is related to documentation. reg:example This is related to the examples. reg:helper-scripts This issue/PR is related to the helper scripts mainly concerned with development of Ginkgo. reg:testing This is related to testing. type:factorization This is related to the Factorizations type:matrix-format This is related to the Matrix formats type:multigrid This is related to multigrid type:preconditioner This is related to the preconditioners type:reordering This is related to the matrix(LinOp) reordering type:solver This is related to the solvers type:stopping-criteria This is related to the stopping criteria
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants