Merge sage_brial into sagelib #30332
Comments
|
comment:1
I propose to copy the current sage-brial code under but other suggestions based on best practices will be given the priority. |
|
comment:2
So there's a Cython interface |
|
comment:3
I thought that sage-brial depends on
Does that answer your question? |
|
comment:4
Thanks. I agree that One thing to keep in mind is the context of the planned modularization of sagelib. |
|
comment:5
On the legal side, the code is GPL2.0 or later. Although I will need to fix the license on github. It seems I changed it from 2+ to 2 carelessly back in 2017 (probably by copying a template). I obviously had no rights to do that. The only thing I may need guidance with is the deprecated re-import. Otherwise moving stuff, including |
|
comment:6
Replying to @kiwifb:
I think GitHub takes the position that there is no such thing as "GPL 2 only": That the license includes "... or any later version" no matter whether you write these words in the files or not. |
|
comment:7
Replying to @mkoeppe:
Very nice of them. But they are not lawyers and I think the "LICENSE" file should be explicit for people who look for these subtle clarification (distros). The GPL FAQ https://www.gnu.org/licenses/old-licenses/gpl-2.0-faq.html#VersionTwoOrLater seem to imply that you should explicitly state it. |
|
comment:8
Tests included inside It looks like that will take much more than just drag and drop in place with a few cosmetic changes. |
|
comment:9
I suppose you could prepare the change to using the sage doctester first, before merging into Sage. |
|
comment:10
Given that sage_brial has no Cython bits, I think it could in fact be removed from the dependencies of sagelib, as it won't really be needed while installing sagelib, only later for docbuild and doctest. |
|
comment:11
Replying to @mkoeppe:
Technically correct. |
|
comment:12
This is not ready for review. I am just pushing my work in progress so it is not just on my workstation, it is visible and open to others contribution. There is a lot to do:
This may take some time and I doubt it will be ready in time for 9.2 but we can only try. New commits:
|
|
Commit: |
|
Branch: public/merge_sage_brial |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:16
I am currently converting docstrings :( I am also doing some selective code deletion as I come along across it. There is a module for memory usage, it is imported once but never used and even less tested. So, I am removing it. It will be worth another look after that first pass. |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
comment:19
I suspect the building of the html doc will break at this stage. If it doesn't the output may very well be useless. Once I figure that out, I'll do the doctesting to see what's broken. I am expecting a lot of breaking in the newly imported tests. |
|
comment:37
Sure, just trying to figure out what the high level interface is intended to be. |
|
comment:38
|
|
comment:39
Right. I remember thinking about that last night. Forgot to remove it. Will do in a few hours if no one beats me to it. |
|
Branch pushed to git repo; I updated commit sha1. New commits:
|
|
Author: François Bissey |
|
comment:43
Replying to @kiwifb:
Indeed, this is a known problem that needs some attention: #21892, #24748, possibly #27019, #23310, #23312, possibly #23311, #15223. My idea is to scrap |
|
comment:44
My idea was to merge this in 9.2 and work on improving things in 9.3. However, if you know exactly what to do, we can work on this now. |
|
comment:46
I think that is a better plan. I am too stretched for time right now to work on this. |
|
Reviewer: Matthias Koeppe |
|
comment:47
I agree with this plan - it will also be a good basis for modularization work in 9.3 |
|
comment:48
I am in the process of looking at adding some of the documentation from sage-brial to the sage documentation. At least to see how it looks like. If you think that's not worth adding now, it is fine. |
|
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:
|
|
comment:50
Just a touch up. One doctest was commented out so I decided to enable it. Building documentation revealed formatting issues in docstrings. All in the same file. I built the doc with some added files from brial and we definitely should keep that for later work. Individual files need sensible title blocks for a start. |
|
Changed branch from public/merge_sage_brial to |
, sagemath#24483, sagemath#24371, sagemath#24511, sagemath#25848, sagemath#26105, sagemath#28481, sagemath#29010, sagemath#29412, sagemath#30332, sagemath#30372, sagemath#31345, sagemath#32375, sagemath#32606, sagemath#32610, sagemath#32612, sagemath#32641, sagemath#32660, sagemath#32750, sagemath#32869, sagemath#33602 <!-- ^^^^^ Please provide a concise, informative and self-explanatory title. Don't put issue numbers in there, do this in the PR body below. For example, instead of "Fixes sagemath#1234" use "Introduce new method to calculate 1+1" --> <!-- Describe your changes here in detail --> <!-- Why is this change required? What problem does it solve? --> <!-- If this PR resolves an open issue, please link to it here. For example "Fixes sagemath#12345". --> <!-- If your change requires a documentation PR, please link it appropriately. --> ### 📝 Checklist <!-- Put an `x` in all the boxes that apply. --> <!-- If your change requires a documentation PR, please link it appropriately --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> <!-- Feel free to remove irrelevant items. --> - [x] The title is concise, informative, and self-explanatory. - [ ] The description explains in detail what this PR is about. - [x] I have linked a relevant issue or discussion. - [ ] I have created tests covering the changes. - [ ] I have updated the documentation accordingly. ### ⌛ Dependencies <!-- List all open PRs that this PR logically depends on - sagemath#12345: short description why this is a dependency - sagemath#34567: ... --> <!-- If you're unsure about any of these, don't hesitate to ask. We're here to help! --> URL: sagemath#36307 Reported by: Matthias Köppe Reviewer(s):
As discussed in #29369 and #28918.
CC: @kiwifb @orlitzky @tscrim
Component: packages: standard
Author: François Bissey
Branch/Commit:
91fe5e1Reviewer: Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/30332
The text was updated successfully, but these errors were encountered: