Opened 4 years ago

Closed 2 years ago

#27171 closed defect (fixed)

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

Reported by: Erik Bray Owned by:
Priority: major Milestone: sage-9.2
Component: refactoring Keywords:
Cc: François Bissey, Timo Kaufmann, John Palmieri, Frédéric Chapoton Merged in:
Authors: Matthias Koeppe Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: da05b3c (Commits, GitHub, GitLab) Commit: da05b3c2df4f2c98cef3ba392187505abd763dd4
Dependencies: #21559 Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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. https://trac.sagemath.org/ticket/27040#comment:48

Change History (15)

comment:1 Changed 4 years ago by François Bissey

Cc: François Bissey added

comment:2 Changed 4 years ago by Timo Kaufmann

Cc: Timo Kaufmann added

comment:3 Changed 4 years ago by Jeroen Demeyer

What do you mean with "live inside the package"?

Version 0, edited 4 years ago by Jeroen Demeyer (next)

comment:4 Changed 4 years ago by Jeroen Demeyer

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?).

comment:5 Changed 4 years ago by Jeroen Demeyer

Description: modified (diff)

comment:6 in reply to:  4 Changed 4 years ago by Erik Bray

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.

Last edited 4 years ago by Erik Bray (previous) (diff)

comment:7 Changed 4 years ago by Erik Bray

Milestone: sage-8.7sage-pending

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

comment:8 Changed 2 years ago by Matthias Köppe

Milestone: sage-pendingsage-9.2
Summary: Move files used by sage at import time to live inside the packageMove file src/bin/sage-maxima.lisp, used by sage at import time, to live inside the package

comment:9 Changed 2 years ago by Matthias Köppe

Dependencies: #21559

comment:10 Changed 2 years ago by Matthias Köppe

Authors: Matthias Koeppe

comment:11 Changed 2 years ago by Matthias Köppe

Branch: u/mkoeppe/move_file_src_bin_sage_maxima_lisp__used_by_sage_at_import_time__to_live_inside_the_package

comment:12 Changed 2 years ago by Matthias Köppe

Cc: John Palmieri Frédéric Chapoton added
Commit: da05b3c2df4f2c98cef3ba392187505abd763dd4
Status: newneeds_review

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

comment:13 Changed 2 years ago by François Bissey

Reviewers: François Bissey
Status: needs_reviewpositive_review

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

comment:14 Changed 2 years ago by Matthias Köppe

Thanks!

comment:15 Changed 2 years ago by Volker Braun

Branch: u/mkoeppe/move_file_src_bin_sage_maxima_lisp__used_by_sage_at_import_time__to_live_inside_the_packageda05b3c2df4f2c98cef3ba392187505abd763dd4
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.