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

Move file src/bin/sage-maxima.lisp, used by sage at import time, to live inside the package #27171

Closed
embray opened this issue Jan 29, 2019 · 14 comments

Comments

@embray
Copy link
Contributor

embray commented Jan 29, 2019

Non-binary files that are part of the sage sources and needed by the sage package at import time should be installed in the package, using package_data in setup.py. See e.g. #27040 comment:48

Depends on #21559

CC: @kiwifb @timokau @jhpalmieri @fchapoton

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: da05b3c

Reviewer: François Bissey

Issue created by migration from https://trac.sagemath.org/ticket/27171

@embray embray added this to the sage-8.7 milestone Jan 29, 2019
@jdemeyer
Copy link

comment:3

(edit: never mind)

@jdemeyer
Copy link

comment:4

Replying to embray:

If it's needed by the sage Python package to work then it should just be installed inside the package (e.g. as package_data in setup.py)

Why should it be installed like that?

I'm not against moving the specific file sage-maxima.lisp, but you seem to imply a more general rule here that Python packages should never access files outside of their own package (or something like that?).

@jdemeyer

This comment has been minimized.

@embray
Copy link
Contributor Author

embray commented Jan 31, 2019

comment:6

Replying to @jdemeyer:

Replying to embray:

If it's needed by the sage Python package to work then it should just be installed inside the package (e.g. as package_data in setup.py)

Why should it be installed like that?

I'm not against moving the specific file sage-maxima.lisp, but you seem to imply a more general rule here that Python packages should never access files outside of their own package (or something like that?).

That's definitely not what I'm saying, though I can see where you get the implication.

This is definitely not a rule in case of files that are not part of Sage, which may come from other packages or be overridden in some way by downstream packagers or by users. In those cases we want an option, with some reasonable default, for where to find that file.

In this case, if I understand correctly, it's just Maxima startup code very specific to Sage's Maxima interface. So there's no reason for it to live anywhere else outside the sage package, and that also makes the question of "where to find it" much simpler, because it's just relative to the package's installation path. And it certainly doesn't belong in any bin/.

Did the same with sage.gaprc in sage.libs.gap.

@embray
Copy link
Contributor Author

embray commented Mar 25, 2019

comment:7

Removing most of the rest of my open tickets out of the 8.7 milestone, which should be closed.

@embray embray removed this from the sage-8.7 milestone Mar 25, 2019
@embray embray added the pending label Mar 25, 2019
@mkoeppe mkoeppe changed the title Move files used by sage at import time to live inside the package Move file src/bin/sage-maxima.lisp, used by sage at import time, to live inside the package Jun 28, 2020
@mkoeppe mkoeppe added this to the sage-9.2 milestone Jun 28, 2020
@mkoeppe mkoeppe removed the pending label Jun 28, 2020
@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

Dependencies: #21559

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

Author: Matthias Koeppe

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

comment:12

1 commit on top of #21559.


Last 10 new commits:

4a3d36eMove 'sage -app' back to src/bin/sage
3a0193csrc/bin/sage: Remove handling of 'sage -axiom'
6b04075Merge branch 't/29111/specify_a_subset_of_sage_command_line_options_that_are_supported_by_sagelib___rather_than_sage_the_distribution' into t/21559/change-src-bin-installation
9c7116bsrc/bin/sage-list-optional, sage-list-experimental, sage-list-standard: Remove deprecated scripts
831cc09Merge branch 't/29920/remove_deprecated_scripts_sage_list_optional__sage_list_experimental__sage_list_standard' into t/21559/change-src-bin-installation
a56dc35Merge tag '9.2.beta1' into t/29702/public/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup
e3eca85Merge branch 'public/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup' of git://trac.sagemath.org/sage into t/21559/change-src-bin-installation
7d29141src/setup.py: Do not install removed script sage-rsyncdist
39cb52aMerge branch 't/21559/change-src-bin-installation' into t/27171/move_file_src_bin_sage_maxima_lisp__used_by_sage_at_import_time__to_live_inside_the_package
da05b3csrc/bin/sage-maxima.lisp: Move inside package, install as package_data

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

Commit: da05b3c

@kiwifb
Copy link
Member

kiwifb commented Jul 4, 2020

Reviewer: François Bissey

@kiwifb
Copy link
Member

kiwifb commented Jul 4, 2020

comment:13

This is sorely needed. I follows the model of other packages for which it was already done. I want this in. LGTM.

@mkoeppe
Copy link
Contributor

mkoeppe commented Jul 4, 2020

comment:14

Thanks!

@vbraun
Copy link
Member

vbraun commented Jul 10, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants