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

Dual-license XML files BSD-2-Clause and CC-BY-4.0 #968

Merged
merged 6 commits into from
Mar 7, 2024
Merged

Conversation

rtwfroody
Copy link
Collaborator

BSD-2-Clause is used so the C code generated from those files can be used in GPLv2 open source projects (specifically, OpenOCD) and the CC-BY-4.0 clause is the standard license for work produced by the RISC-V Foundation.

@rtwfroody rtwfroody requested a review from rpsene February 13, 2024 17:55
Copy link
Contributor

@rpsene rpsene left a comment

Choose a reason for hiding this comment

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

@rtwfroody LGTM.

@rtwfroody
Copy link
Collaborator Author

@borneoa does this work for you?

@borneoa
Copy link

borneoa commented Feb 21, 2024

Few issues in the generated header for the C files:

The first point should be a legal requirement, while the other two are highly welcome but can still be managed through a manual editing of the files before integration in another project like OpenOCD

BSD-2-Clause is used so the C code generated from those files can be
used in GPLv2 open source projects (specifically, OpenOCD) and the
CC-BY-4.0 clause is the standard license for work produced by the RISC-V
Foundation.
Also multiple identifiers must be on the same line together.
@rtwfroody
Copy link
Collaborator Author

  • the correct SPDX tag should be SPDX-License-Identifier: CC-BY-4.0 OR BSD-2-Clause in a single line, or accordingly to https://wiki.spdx.org/view/SPDX_FAQ it should be SPDX-License-Identifier: (CC-BY-4.0 or BSD-2-Clause)

Fixed.

  • the SPDX tag should be in the very first line of the file, to help automatic tools to build the SW BOM

Fixed.

I've left this as-is. /* */ works in every C compiler. // does not (although you might have to go back a way to find a compiler that doesn't accept //).

Copy link
Collaborator

@pdonahue-ventana pdonahue-ventana left a comment

Choose a reason for hiding this comment

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

// comments were standardized in regular C in 1999, but using old-school comments is fine and will make C89 people happy.

I was going to advise against using Python f-strings for a similar reason since that requires Python 3.6. However, I see that registers.py already uses f-strings extensively (which nobody seems to be complaining about) and 3.6 is now seven years old. So that's fine. And f-strings are so much nicer than the old clunky ways.

@borneoa
Copy link

borneoa commented Feb 24, 2024

If I remember correctly, the SPDX spec was requiring to use a "single line comment" so the decision was to use // comment style. But this was not compatible with files.h included by assembly files.
At the end, // comment was kept for files.c only, while the backward compatible /* comment */ was used in files.h.
Anyway, not a big issue; let's keep this simple.

The result is now good for me.

There is one new issue with build/Makefile, commented in the patch review

Copy link

@borneoa borneoa left a comment

Choose a reason for hiding this comment

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

build issue in this commit

@@ -70,14 +70,9 @@ build-registers: $(REGISTERS_ADOC)
../registers.py --adoc $@ --adoc-definitions $(patsubst %.adoc,%-def.adoc,$@) $<

debug_defines: debug_defines.h debug_defines.c $(patsubst %,../xml/%,$(REGISTERS_ADOC:.adoc=.xml)) ../registers.py
../registers.py $@ --cgetters $(filter %.xml, $^)

debug_defines.%:
Copy link

Choose a reason for hiding this comment

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

By removing this make's target debug_defines.% the build make debug_defines fails on clean build, e.g. after a clean git clone
You need to drop debug_defines.h and debug_defines.c at line 74

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Fixed.

build/Makefile Outdated
@@ -70,14 +70,9 @@ build-registers: $(REGISTERS_ADOC)
../registers.py --adoc $@ --adoc-definitions $(patsubst %.adoc,%-def.adoc,$@) $<

debug_defines: debug_defines.h debug_defines.c $(patsubst %,../xml/%,$(REGISTERS_ADOC:.adoc=.xml)) ../registers.py
Copy link

Choose a reason for hiding this comment

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

Need to remove the dependencies debug_defines.h and debug_defines.c because the files are not added anymore after line 75 has been removed

The rule makes those targets from scratch now.
@borneoa
Copy link

borneoa commented Feb 26, 2024

I'm fine with it. Thanks

@rtwfroody rtwfroody added the 1.0 label Mar 7, 2024
@rtwfroody rtwfroody merged commit c520fb3 into main Mar 7, 2024
1 check passed
@rtwfroody rtwfroody deleted the license branch March 7, 2024 17:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants