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

Add api doc #2206

Merged
merged 10 commits into from Jan 5, 2020
Merged

Add api doc #2206

merged 10 commits into from Jan 5, 2020

Conversation

@SchoolGuy
Copy link
Contributor

SchoolGuy commented Nov 15, 2019

To also publish the code docs we can just enable sphinx. This also includes an upgrade for sphinx and the conf files.

@SchoolGuy SchoolGuy added the doc label Nov 15, 2019
@SchoolGuy SchoolGuy self-assigned this Nov 15, 2019
@SchoolGuy SchoolGuy added this to Pull Requests in Backlog via automation Nov 15, 2019
@SchoolGuy SchoolGuy force-pushed the openSUSE:add-api-doc branch from dc14556 to 04195c3 Nov 20, 2019
@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Nov 20, 2019

Currently a lot of the autodoc sections are empty because of this stactrace...

'tftpgen' from module 'cobbler'; the following exception was raised:
Traceback (most recent call last):
  File "/home/egotthold/Sources/Pycharm Projects/cobbler/venv/lib/python3.7/site-packages/sphinx/ext/autodoc/importer.py", line 32, in import_module
    return importlib.import_module(modname)
  File "/usr/lib/python3.7/importlib/__init__.py", line 127, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1006, in _gcd_import
  File "<frozen importlib._bootstrap>", line 983, in _find_and_load
  File "<frozen importlib._bootstrap>", line 967, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 677, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 728, in exec_module
  File "<frozen importlib._bootstrap>", line 219, in _call_with_frames_removed
  File "/home/egotthold/Sources/Pycharm Projects/cobbler/cobbler/tftpgen.py", line 32, in <module>
    from cobbler import templar
  File "/home/egotthold/Sources/Pycharm Projects/cobbler/cobbler/templar.py", line 44, in <module>
    from .template_api import Template
  File "/home/egotthold/Sources/Pycharm Projects/cobbler/cobbler/template_api.py", line 72, in <module>
    MacrosTemplate = Cheetah.Template.Template.compile(file=CHEETAH_MACROS_FILE)
  File "/home/egotthold/Sources/Pycharm Projects/cobbler/venv/lib/python3.7/site-packages/Cheetah/Template.py", line 738, in compile
    fileHash += str(os.path.getmtime(file))
  File "/usr/lib/python3.7/genericpath.py", line 55, in getmtime
    return os.stat(filename).st_mtime
FileNotFoundError: [Errno 2] Datei oder Verzeichnis nicht gefunden: '/etc/cobbler/cheetah_macros'

Since this is related to docs this will also be fixed inside this PR. The file cheetah_macros is living in this repo under config/cheetah/cheetah_macros. The path given in the stacktrace is only valid on a packaged installation of cobbler.

@watologo1 watologo1 marked this pull request as ready for review Nov 21, 2019
@watologo1

This comment has been minimized.

Copy link
Contributor

watologo1 commented Nov 21, 2019

Oh, I wanted to have a look at the whole patchset and hit "ready for review" (more interpreted the button as "review" only).
I triggered something by this as the patch can now be merged, but before I mess this up even more, I better leave this to Enno. Sry for any inconvenience.

@SchoolGuy SchoolGuy mentioned this pull request Dec 12, 2019
Copy link
Contributor

kwizart left a comment

Why the license headers removal ?

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Dec 16, 2019

@kwizart Because we have a copying in the file root and thus the headers are then just not needed anymore.

@kwizart

This comment has been minimized.

Copy link
Contributor

kwizart commented Dec 16, 2019

Well, I've never saw that anywhere that copyright headers can be removed by another party.
Please have a look at any single file in the kernel.org is there a missing license header ?

As a Fedora packager maintainer our guidelines is pretty clear that we mandate a license header for every single file provided by a FLOSS project. Please check with your legal service, you do not have right to remove the copyright headers.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Dec 17, 2019

@kwizart So until I may get a response from our legal guys I found this stackexchange post for example: https://softwareengineering.stackexchange.com/questions/19649/copyright-notices-disclaimers-in-source-files

@brejoc

This comment has been minimized.

Copy link
Member

brejoc commented Jan 2, 2020

Well, I've never saw that anywhere that copyright headers can be removed by another party.
Please have a look at any single file in the kernel.org is there a missing license header ?

What do you mean with an other party? There is no license change and @SchoolGuy is the project owner. So this is about organizing the sources. I also don't see why you pointed to the kernel. That's an other project and they decided to do it that way.

As a Fedora packager maintainer our guidelines is pretty clear that we mandate a license header for every single file provided by a FLOSS project. Please check with your legal service, you do not have right to remove the copyright headers.

Okay, that's more interesting. Do you have a link to that policy at hand? We certainly don't want to complicate life of Fedora package maintainers. But this bears an other question: What about projects that don't have a license header in each and every file? Do you add those?

@SchoolGuy will check with legal. But until there is an answer, here is a quote from gnu.org:

To do so, attach the following notices to the program. It is safest to attach them to the start of each source file to most effectively convey the exclusion of warranty; and each file should have at least the "copyright" line and a pointer to where the full notice is found.

Source: http://www.gnu.org/licenses/old-licenses/gpl-2.0.html

"It is safest" doesn't mean that you have to do it. You just have to have the license file with the sources and if there are mixed licenses you have to make sure that it is understandable which file is license under which license. So it could be, that the Fedora packaging guidelines are more strict than what the GNU requires.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

After talking to one of our legal guys I can now say that the project owner decides what convention we may follow. Since I am the project owner and the LICENSE headers are bloating the docs after this PR and the license is clearly stated in the LICENSE file, I will let commit 8fdbc47 stay in this PR and thus in the future in this repository. @kwizart

CC: @brejoc @watologo1 @Conan-Kudo

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Jan 2, 2020

@SchoolGuy You really, really, really should ask for permission from the folks mentioned in those license headers before removing them. Both the Linux kernel and systemd requested approval from the folks mentioned in each copyright header and the copyright holders before doing so.

That's why, for example, my copyright header attributions still remain in the rpm macros and triggers files in the systemd project. I never gave my assent to strip it, so they remain.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

@Conan-Kudo Is there any maximum wait time for response in your eyes? Because I doubt that all people mentioned in these files will answer.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Jan 2, 2020

@SchoolGuy I'd probably suggest a month. But this is one of those things where if people don't respond, it's probably better to leave it alone (up to you though).

I suggest you put together a central AUTHORS if you want to do this, too.

That said, one of the reasons that Red Hat and other organizations put per-file license headers is so that when files get copied around, it's more obvious when someone may have either ignored the terms of the license willfully or something else along those lines. I don't disagree with this rationale, but I'm not the project owner...

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

@Conan-Kudo Okay thanks also to you for your input. The AUTHORs file is currently generated manually via the Makefile which has not happend for a while now.

My personal opinion is that if someone willingly wants to violate the terms of our license a license header will not stop that person. If someone is not doing it on purpose we will be able to reason with them without upsetting any of the parties involved.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

Note the follwing people need to be contacted either via Github (optinally) and via mail.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

If any of these people mentioned above sees this please state if you are okay with the removal of the License header and instead adding you to the AUTHORS file.

@kwizart

This comment has been minimized.

Copy link
Contributor

kwizart commented Jan 2, 2020

@SchoolGuy I'd probably suggest a month.

Unfortunately, this is not an area anyone else than the copyright holder can intervene.
Unless the code is "signed-off" (1), copyright holder decision is mandated.

Now I'm not sure what the reason behind this copyright notice removal. If it's about to get the code header shorter, then it's probably better to move the GPL notice to use the SPDX identifier instead which is a one liner and keep the Copyright line.
This wouldn't need any author acceptance, wouldn't need any legal clearance and would be a easier way forward.

After talking to one of our legal guys...

Sorry, but I don't know what the question to their answer was. If you are talking to a new project from scratch, I'm pretty sure the short "answer" you have reported apply.
Now I keep saying that for existing code, you cannot remove the copyright line without having the copyright holder (aka the author most of the time) to consent.

(1) See also https://www.kernel.org/doc/html/v5.4/process/submitting-patches.html

@p3ck

This comment has been minimized.

Copy link
Contributor

p3ck commented Jan 2, 2020

I'm ok with the change. Thanks for checking.

@jeckersb

This comment has been minimized.

Copy link
Contributor

jeckersb commented Jan 2, 2020

I'm ok with the change. Thanks for checking.

Same here, thanks folks!

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

Now I'm not sure what the reason behind this copyright notice removal. If it's about to get the code header shorter, then it's probably better to move the GPL notice to use the SPDX identifier instead which is a one liner and keep the Copyright line.
This wouldn't need any author acceptance, wouldn't need any legal clearance and would be a easier way forward.

Yes this is exactly what I want because this will be in full length in the docs then and this is not pretty when reading docs. A one liner would be a very good compromise I would be very happy with. Since my knowledge about licenses is limited are you suggesting the following? - https://spdx.org/ids-how @kwizart

@SEJeff

This comment has been minimized.

Copy link
Contributor

SEJeff commented Jan 2, 2020

@SchoolGuy I’m ok with this change. You’re not changing the license, just the way the license is specified in this project.

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

I'd also like to point out that I was removed from the organization when other folks who are not contributing are still here . Previously discussed with @brejoc - didn't see anything change, and it should be consistent one way or the other.

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

@mpdehaan Invitation was sent

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

You've also erased me from the README, which also needs to be corrected.

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

Thanks @SchoolGuy

@kwizart

This comment has been minimized.

Copy link
Contributor

kwizart commented Jan 2, 2020

... you suggesting the following? - https://spdx.org/ids-how

So indeed, that's probably the way forward (along with keeping the Copyright lines)

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

You've also erased me from the README, which also needs to be corrected.

Forgive my stupidity but if I looked correctly through the history of the README.md file your name there was never in it.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Jan 2, 2020

I personally like the variant that @sgallagher is using with libmodulemd:

# This file is part of libmodulemd
# Copyright (C) 2016 Red Hat, Inc.
# Copyright (C) 2017-2018 Stephen Gallagher
#
# Fedora-License-Identifier: MIT
# SPDX-2.0-License-Identifier: MIT
# SPDX-3.0-License-Identifier: MIT
#
# This program is free software.
# For more information on the license, see COPYING.
# For more information on free software, see
# <https://www.gnu.org/philosophy/free-sw.en.html>.

In our case, these would be our identifiers:

# Fedora-License-Identifier: GPLv2+
# SPDX-2.0-License-Identifier: GPL-2.0+
# SPDX-3.0-License-Identifier: GPL-2.0-or-later
@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 2, 2020

Okay since Michael voted against the removal of his name in the specific files we won't remove the headers. I will remove the corresponding commit and we will sort out how we do this in the future in another PR/Issue/Gitter. Let's don't fill this PR up anymore than it is now.

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

@SchoolGuy - My apologies, you are correct on that front and I see it is in AUTHORS.in still. Was just checking things through.

Possibly important though - For some reason we are missing some Git history, but Cobbler I believe used to be in Mercurial, so while README goes back to 2015, the history on README should go back to 2006. I am probably responsible for that missing data in the git import from hg dropping some history, but who knows. My point is that it is very likely that many historical AUTHORS aren't going to be picked up by AUTHORS.in in the current method at all, and if that is what you are going on for sending emails, you'll miss some people. I was not around for the "AUTHORS.in" change and I'm sure whoever did it didn't think about the lost history in deleting the original list. I do understand now that, these days, people typically don't keep a list, but they do keep history forever.

This means that we can't say "copyright people in the AUTHORS file" - which is right now, also, not checked into git. So the copyright lines need to be what they were before.

Cobbler, Func, and Ansible were somewhat designed such that the license was really hard to change, as a way of encouraging people to contribute knowing that would always be the case. There never was any copyright assignment or CLA, in other words. Those kinds of things would have provided a more obvious path to changing those headers.

That being said, I don't really care about the license text moving to what Neal proposed above as long as no copyright lines are adjusted and we aren't deleting people's names or contact info anywhere, and they still appear in every file and there's a COPYING or LICENSE file, etc.

(GPLv2 vs MIT, obviously)

@mpdehaan

This comment has been minimized.

Copy link
Contributor

mpdehaan commented Jan 2, 2020

@SchoolGuy

"Okay since Michael voted against the removal of his name in the specific files we won't remove the headers"

To clarify, I'm against removal of anyone's names (COPYRIGHT), I'm fine (aka "I don't care") with the rest of it using the headers (LICENSE).

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Jan 2, 2020

@SchoolGuy If it's just about reducing boilerplate, I think what I've proposed should be fine.

@watologo1

This comment has been minimized.

Copy link
Contributor

watologo1 commented Jan 2, 2020

Ideally you simply revert any author or license changes.

In the kernel they shortened all "full GPL headers" in every file to SPDX shortcuts as suggested above already. But is this change really worth it? I'd just keep this.

Removing authors is certainly a no-go.

My 2 cents...

@SchoolGuy SchoolGuy force-pushed the openSUSE:add-api-doc branch from 926c77c to b7ab635 Jan 3, 2020
@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 3, 2020

Rebase onto master to eliminate the possibility that the packaging PR conflicts in any way.

@shenson

This comment has been minimized.

Copy link

shenson commented Jan 5, 2020

I'm good with this change. Could you update the email address to scott@foolishpride.org as well please.

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

Conan-Kudo commented Jan 5, 2020

@SchoolGuy you should use git rebase to drop the license headers commits (both the original and the revert commit).

@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 5, 2020

@Conan-Kudo I expect that the tests pass now. I will then do so.

SchoolGuy added 10 commits Sep 26, 2019
@SchoolGuy SchoolGuy force-pushed the openSUSE:add-api-doc branch from 71962c8 to 3809bbb Jan 5, 2020
@SchoolGuy

This comment has been minimized.

Copy link
Contributor Author

SchoolGuy commented Jan 5, 2020

Okay so after the rebase there is no commit touching license headers anymore. This PR adds the auto-doc for the cobbler code and fixes some problems with the Cheetah usage which were needed to be fixed inside this PR because otherwise rtd would not build the docs.

I will now bring this into master and fix the actual content content of the docs in another PR as well as exchanging the headers to the new format we all agreed on. There was no negative comment on the docs changes during the discussion that's the reason why I won't wait for approval this time.

@SchoolGuy SchoolGuy changed the title WIP: Add api doc Add api doc Jan 5, 2020
@SchoolGuy SchoolGuy merged commit 7a5d6cc into cobbler:master Jan 5, 2020
3 checks passed
3 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-documentation/read-the-docs Read the Docs build succeeded!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Backlog automation moved this from Pull Requests to Done Jan 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Backlog
  
Done
10 participants
You can’t perform that action at this time.