Opened 4 years ago
Closed 2 years ago
#27171 closed defect (fixed)
Move file src/bin/sagemaxima.lisp, used by sage at import time, to live inside the package
Reported by:  embray  Owned by:  

Priority:  major  Milestone:  sage9.2 
Component:  refactoring  Keywords:  
Cc:  fbissey, ghtimokau, jhpalmieri, 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: 
Description (last modified by )
Nonbinary 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
 Cc fbissey added
comment:2 Changed 4 years ago by
 Cc ghtimokau added
comment:3 Changed 4 years ago by
comment:4 followup: ↓ 6 Changed 4 years ago by
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
insetup.py
)
Why should it be installed like that?
I'm not against moving the specific file sagemaxima.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
 Description modified (diff)
comment:6 in reply to: ↑ 4 Changed 4 years ago by
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
insetup.py
)Why should it be installed like that?
I'm not against moving the specific file
sagemaxima.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
.
comment:7 Changed 3 years ago by
 Milestone changed from sage8.7 to sagepending
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
 Milestone changed from sagepending to sage9.2
 Summary changed from Move files used by sage at import time to live inside the package to Move file src/bin/sagemaxima.lisp, used by sage at import time, to live inside the package
comment:9 Changed 2 years ago by
 Dependencies set to #21559
comment:10 Changed 2 years ago by
comment:11 Changed 2 years ago by
 Branch set to 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
 Cc jhpalmieri chapoton added
 Commit set to da05b3c2df4f2c98cef3ba392187505abd763dd4
 Status changed from new to needs_review
1 commit on top of #21559.
Last 10 new commits:
4a3d36e  Move 'sage app' back to src/bin/sage

3a0193c  src/bin/sage: Remove handling of 'sage axiom'

6b04075  Merge branch 't/29111/specify_a_subset_of_sage_command_line_options_that_are_supported_by_sagelib___rather_than_sage_the_distribution' into t/21559/changesrcbininstallation

9c7116b  src/bin/sagelistoptional, sagelistexperimental, sageliststandard: Remove deprecated scripts

831cc09  Merge branch 't/29920/remove_deprecated_scripts_sage_list_optional__sage_list_experimental__sage_list_standard' into t/21559/changesrcbininstallation

a56dc35  Merge tag '9.2.beta1' into t/29702/public/move_all_code_from_src_setup_py__src_fpickle_setup_py_to_sage_setup

e3eca85  Merge 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/changesrcbininstallation

7d29141  src/setup.py: Do not install removed script sagersyncdist

39cb52a  Merge branch 't/21559/changesrcbininstallation' into t/27171/move_file_src_bin_sage_maxima_lisp__used_by_sage_at_import_time__to_live_inside_the_package

da05b3c  src/bin/sagemaxima.lisp: Move inside package, install as package_data

comment:13 Changed 2 years ago by
 Reviewers set to François Bissey
 Status changed from needs_review to positive_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
Thanks!
comment:15 Changed 2 years ago by
 Branch changed from u/mkoeppe/move_file_src_bin_sage_maxima_lisp__used_by_sage_at_import_time__to_live_inside_the_package to da05b3c2df4f2c98cef3ba392187505abd763dd4
 Resolution set to fixed
 Status changed from positive_review to closed
(edit: never mind)