Opened 3 years ago

Closed 3 years ago

#25039 closed enhancement (fixed)

Add sdh_install helper function to sage-dist-helpers

Reported by: embray Owned by:
Priority: major Milestone: sage-8.3
Component: build Keywords: destdir mathjax thebe
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: 0f848c3 (Commits, GitHub, GitLab) Commit: 0f848c3153b06b42839e908a5f3324084a604ac9
Dependencies: Stopgaps:

Status badges

Description

Adds a new sdh_install helper function:

This is like a very simplified version of the install program, and is intended for use by packages whose spkg-install just copies some files (or more complex packages that copy some files as one of its steps). The majority of its functionality is just in ensuring that the full destination path exists, and to provide some error checking. This replaces some patterns that occur frequently in some spkg-installs.

Also updates the mathjax and thebe packages as two examples of its usage (this implements #24024 for those packages).

Change History (17)

comment:1 Changed 3 years ago by embray

  • Status changed from new to needs_review

comment:2 Changed 3 years ago by git

  • Commit changed from 6a4e33a7b5c4b3e0d868cfd4f060af043134b5f7 to 22c3e9eb0fbb1147caf19c4d9219c54b5187308a

Branch pushed to git repo; I updated commit sha1. New commits:

330e618Introduce SAGE_DESTDIR_LOCAL variable
9d14972Don't install gcc as part of gfortran
5f8549eMerge branch 'u/jdemeyer/gfortran_breaks_parallel_build' into u/embray/build/sdh_install_files
22c3e9efix bug in handling of the -T flag--it was as if it was always set

comment:3 Changed 3 years ago by git

  • Commit changed from 22c3e9eb0fbb1147caf19c4d9219c54b5187308a to 6dcb88027d02ac882cf9dba990a43c4894a91f85

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

6c8bd46fix bug in handling of the -T flag--it was as if it was always set
6dcb880fix bug that can occur in repeated invocations of sdh_install in the same script

comment:4 follow-up: Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work
  1. I don't really understand the point of
        if [ $# -eq 0 ]; then
            sdh_die "At least a source and destination file are required"
        fi
    

since it would fail anyway later and no error message would be given for sdh_install -T.

  1. It looks strange to put this in an else block, it's just part of the normal flow of execution:
        else
            dest="${SAGE_DESTDIR}$dest"
        fi
    
  1. I seem to recall that cp -R is more portable than cp -r. So unless you have a good reason for using cp -r, I recommend cp -R.
  1. For consistency, I would start all sdh_die messages with Error:
  1. echo "Installing files for ${PKG_NAME}:" looks like unneeded verbosity. I would drop that.
  1. If mkdir or cp fails, it already prints an error message, so I don't need that we need sdh_die for that. I would just write cp ... || exit $?.

comment:5 in reply to: ↑ 4 Changed 3 years ago by embray

Replying to jdemeyer:

  1. I don't really understand the point of
        if [ $# -eq 0 ]; then
            sdh_die "At least a source and destination file are required"
        fi
    

since it would fail anyway later and no error message would be given for sdh_install -T.

I mean, it will still fail for sdh_install -T because the $src variable will be empty. I thought an explicit check of "there are arguments" would enhance clarity but in retrospect I agree it's superfluous.

  1. It looks strange to put this in an else block, it's just part of the normal flow of execution:
        else
            dest="${SAGE_DESTDIR}$dest"
        fi
    

Yep. I think that was just the result of remolding of slightly different logic.

  1. I seem to recall that cp -R is more portable than cp -r. So unless you have a good reason for using cp -r, I recommend cp -R.

I suspect it's not really a problem for us since I've already seen cp -r in some of the spkg-installs I've used this in--including standard packages. If that were a problem for a platform we care about it would have come up by now. However, I seem to recall the same now that you mention it, so I could change it.

  1. For consistency, I would start all sdh_die messages with Error:

+1

  1. echo "Installing files for ${PKG_NAME}:" looks like unneeded verbosity. I would drop that.

+1 Some of the other sdh_ commands had some kind of message like this so I put it in for consistency. But after trying this on a few packages it hasn't been too helpful (especially in cases where there are multiple sdh_install calls in the package).

  1. If mkdir or cp fails, it already prints an error message, so I don't need that we need sdh_die for that. I would just write cp ... || exit $?.

A lot of the code this function is replacing was also adding its own error messages on top of mkdir and/or cp, so this was just carried over from that. In most cases those programs' error messages should be sufficient though.

comment:6 Changed 3 years ago by git

  • Commit changed from 6dcb88027d02ac882cf9dba990a43c4894a91f85 to 42726a27a347184a9e55b24f2f0d599855b92933

Branch pushed to git repo; I updated commit sha1. New commits:

42726a2various minor review comments

comment:7 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:8 follow-up: Changed 3 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

Wait... did you actually agree with all my comments? :-)

comment:9 in reply to: ↑ 8 Changed 3 years ago by embray

Replying to jdemeyer:

Wait... did you actually agree with all my comments? :-)

Fair enough--but it's not like I set out to disagree with you. I thought they were all reasonable enough :)

comment:10 Changed 3 years ago by jdemeyer

  • Branch changed from u/embray/build/sdh_install_files to u/jdemeyer/build/sdh_install_files

comment:11 Changed 3 years ago by jdemeyer

  • Commit changed from 42726a27a347184a9e55b24f2f0d599855b92933 to fa991ca00d7f16946a4753acb145d0ebd2a53857

New commits:

fa991casdh_install_files -> sdh_install in error messages

comment:12 Changed 3 years ago by embray

Thanks for catching that. It was originally called sdh_install_files but I decided to shorten it later.

comment:13 Changed 3 years ago by embray

  • Branch changed from u/jdemeyer/build/sdh_install_files to u/embray/build/sdh_install_files
  • Commit changed from fa991ca00d7f16946a4753acb145d0ebd2a53857 to dbd9ca6aad7047849547359ee9b8c06b05413a09
  • Milestone changed from sage-8.2 to sage-8.3

Just rebased on current develop.


New commits:

27a44b4Adds a new sdh_install helper function:
0bbff00fix bug in handling of the -T flag--it was as if it was always set
7d09c66fix bug that can occur in repeated invocations of sdh_install in the same script
598112bvarious minor review comments
dbd9ca6sdh_install_files -> sdh_install in error messages

comment:14 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:15 Changed 3 years ago by git

  • Commit changed from dbd9ca6aad7047849547359ee9b8c06b05413a09 to 0f848c3153b06b42839e908a5f3324084a604ac9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b0b38c2Adds a new sdh_install helper function:
852cb5ffix bug in handling of the -T flag--it was as if it was always set
5f7cd30fix bug that can occur in repeated invocations of sdh_install in the same script
daa37d5various minor review comments
0f848c3sdh_install_files -> sdh_install in error messages

comment:16 Changed 3 years ago by embray

  • Status changed from needs_work to positive_review

comment:17 Changed 3 years ago by vbraun

  • Branch changed from u/embray/build/sdh_install_files to 0f848c3153b06b42839e908a5f3324084a604ac9
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.