Opened 3 years ago

Closed 17 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, GitHub, GitLab) Commit: da05b3c2df4f2c98cef3ba392187505abd763dd4
Dependencies: #21559 Stopgaps:

Status badges

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 3 years ago by fbissey

  • Cc fbissey added

comment:2 Changed 3 years ago by gh-timokau

  • Cc gh-timokau added

comment:3 Changed 3 years ago by jdemeyer

(edit: never mind)

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:4 follow-up: Changed 3 years 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 3 years ago by jdemeyer

  • Description modified (diff)

comment:6 in reply to: ↑ 4 Changed 3 years 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 3 years ago by embray (previous) (diff)

comment:7 Changed 3 years 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 18 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 17 months ago by mkoeppe

  • Dependencies set to #21559

comment:10 Changed 17 months ago by mkoeppe

  • Authors set to Matthias Koeppe

comment:11 Changed 17 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 17 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 17 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 17 months ago by mkoeppe

Thanks!

comment:15 Changed 17 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.