-
-
Notifications
You must be signed in to change notification settings - Fork 568
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
Move cremona db to features #25825
Comments
comment:3
I just discovered #24718. This seems to be related/a duplicate. But that ticket seems to do some more unrelated stuff and probably needs a rebase. No clue why that is still "new". |
comment:4
In any case, I would rather have 1 ticket for each issue (1 for GAP, one for the Cremona database). |
comment:5
Sure, I'll split it up. |
New commits:
|
Changed branch from u/gh-timokau/use-features to u/gh-timokau/cremona-db-features |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
This comment has been minimized.
This comment has been minimized.
comment:8
Split to this and #25835. |
comment:9
As I mentioned on #24718: We should also use the database location discovered by the feature check instead of hardcoding it. In other words: there should be a single source for the database location. |
Dependencies: #25309 |
comment:10
What do you mean by "discover"? It looks to me like the path to the database is just as hardcoded in the feature. Am I reading that wrong? That should probably use the environment variable I introduced in #25309 though. So that'll have to be merged first. |
comment:11
Replying to @timokau:
Yes, it's hardcoded in two different places which is bad design. There should be single source for it. |
comment:12
I agree, I'll use the environment variable after #25309 is merged. |
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
|
comment:15
Alright I'm using the proper environment variables now. |
comment:51
The last 2 commits are not strictly needed, they are just cleaning stuff up. If a reviewer prefers to move that to a separate ticket, I'll do that. |
comment:52
I am just verifying the last two cleanup commits are good. |
comment:53
Rebase needed |
Changed branch from u/jdemeyer/cremona-db-features to u/arojas/cremona-db-features |
Changed author from Timo Kaufmann, Jeroen Demeyer to Timo Kaufmann, Jeroen Demeyer, Antonio Rojas |
comment:56
Ticket description needs updating |
comment:57
Replying to @mkoeppe:
Is the |
comment:58
No, the use of |
This comment has been minimized.
This comment has been minimized.
Changed reviewer from Julian Rüth to Julian Rüth, Matthias Koeppe |
This comment has been minimized.
This comment has been minimized.
Changed branch from u/arojas/cremona-db-features to |
This continues the work in #20382 and #25835.
To test this I ran
./sage -t --optional=sage,gap_packages,database_cremona_ellcurve $( git diff --name-only HEAD~2 )
.Next steps in getting rid of
is_package_installed
etc.:sage.doctest.control
: Replace use ofsage.misc.package.list_packages
sage.misc.package
except forPackageNotFoundError
CC: @tscrim @kiwifb @mkoeppe
Component: build
Author: Timo Kaufmann, Jeroen Demeyer, Antonio Rojas
Branch/Commit:
9c2dcb5
Reviewer: Julian Rüth, Matthias Koeppe
Issue created by migration from https://trac.sagemath.org/ticket/25825
The text was updated successfully, but these errors were encountered: