Opened 22 months ago

Closed 5 months 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: embray Owned by:
Priority: major Milestone: sage-9.2
Component: refactoring Keywords:
Cc: fbissey, gh-timokau, jhpalmieri, chapoton Merged in:
Authors: Matthias Koeppe Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: da05b3c (Commits) Commit: da05b3c2df4f2c98cef3ba392187505abd763dd4
Dependencies: #21559 Stopgaps:

Description (last modified by jdemeyer)

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 22 months ago by fbissey

  • Cc fbissey added

comment:2 Changed 22 months ago by gh-timokau

  • Cc gh-timokau added

comment:3 Changed 22 months ago by jdemeyer

(edit: never mind)

Last edited 22 months ago by jdemeyer (previous) (diff)

comment:4 follow-up: Changed 22 months ago by 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?).

comment:5 Changed 22 months ago by jdemeyer

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Changed 22 months ago by embray

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 22 months ago by embray (previous) (diff)

comment:7 Changed 21 months ago by embray

  • Milestone changed from sage-8.7 to sage-pending

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

comment:8 Changed 5 months ago by mkoeppe

  • Milestone changed from sage-pending to sage-9.2
  • Summary changed from Move files used by sage at import time to live inside the package to Move file src/bin/sage-maxima.lisp, used by sage at import time, to live inside the package

comment:9 Changed 5 months ago by mkoeppe

  • Dependencies set to #21559

comment:10 Changed 5 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:11 Changed 5 months ago by mkoeppe

  • 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 5 months ago by mkoeppe

  • 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:

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 5 months ago by fbissey

  • 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 5 months ago by mkoeppe

Thanks!

comment:15 Changed 5 months ago by vbraun

  • 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
Note: See TracTickets for help on using tickets.