Opened 10 years ago

Closed 10 years ago

#11663 closed defect (fixed)

singular files not world-readable

Reported by: jdemeyer Owned by: tbd
Priority: blocker Milestone: sage-4.7.1
Component: packages: standard Keywords:
Cc: Merged in: sage-4.7.1.rc2
Authors: Jeroen Demeyer Reviewers: Leif Leonhardy
Report Upstream: Reported upstream. Little or no feedback. Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Change History (28)

comment:1 Changed 10 years ago by leif

Note that there's already a p10 (#11550), and trivial outstanding changes to that (#11645).

comment:2 Changed 10 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:3 Changed 10 years ago by jdemeyer

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.

comment:4 Changed 10 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. Little or no feedback.

comment:5 follow-up: Changed 10 years ago by leif

Which (of the installed) files were affected?

I only noticed

local/share/singular/singular.hlp
local/share/singular/singular.idx

(and local/share/singular/help.cnf, which currently unintentionally doesn't get installed, cf. #11519 and #5994).

Also, all (header) files in local/include/singular are executable. :)

comment:6 Changed 10 years ago by leif

Note that (almost*) all files are installed with some install script or program (not consistently making use of -m ... though as far as I can see), so changing file permissions in src/ isn't really appropriate or may not be sufficient.


*There are actually a few (at least one) instances where cp is used.

comment:7 in reply to: ↑ 5 ; follow-up: Changed 10 years ago by leif

Replying to leif:

I only noticed

local/share/singular/singular.hlp
local/share/singular/singular.idx

(and local/share/singular/help.cnf, which currently unintentionally doesn't get installed, cf. #11519 and #5994).

These two files are actually copied by spkg-install:

install_docs()
{
    cp $SHARED/singular.hlp $SAGE_LOCAL/share/singular/
    if [ $? -ne 0 ]; then
        echo "Error installing documentation while copying singular.hlp"
        exit 1
    fi
    cp $SHARED/singular.idx $SAGE_LOCAL/share/singular/
    if [ $? -ne 0 ]; then
        echo "Error installing documentation while copying singular.idx"
        exit 1
    fi
}

So it's not really an upstream issue.

comment:8 Changed 10 years ago by leif

P.S.: The created Singular scripts in local/bin/ use $* instead of "$@".

comment:9 follow-up: Changed 10 years ago by leif

  • Reviewers set to Leif Leonhardy
  • Status changed from needs_review to needs_work
  • Work issues set to Use `cp -p` in `spkg-install`

This doesn't work (i.e., doesn't solve the issue), since cp is required (by POSIX) to respect the umask unless -p is used (which we don't, see above).

Also, janet.{cc,h} are still executable in the upstream sources, and at least for me all headers still get installed with -rwxr-xr-x.

While you're at it (spkg-install), I think you should also add copying the help.cnf file (which is as noted missing in our "manual" installations).

comment:10 in reply to: ↑ 9 Changed 10 years ago by leif

Replying to leif:

While you're at it (spkg-install), I think you should also add copying the help.cnf file (which is as noted missing in our "manual" installations).

Please also quote some environment variables (at least there, in install_docs()).

comment:11 Changed 10 years ago by leif

Instead of using cp -p, you could of course also chmod them afterwards, or simply change the umask before copying.

In principle, we could (or should?) set the umask in sage-spkg.

comment:12 Changed 10 years ago by leif

Actually, the problem with singular.{hlp,idx} is not my umask, but that the files already existed (and don't get deleted prior to reinstallation), so their permissions didn't get changed.

Anyway, using cp -p would solve this.

comment:13 Changed 10 years ago by jdemeyer

  • Description modified (diff)

comment:14 in reply to: ↑ 7 Changed 10 years ago by jdemeyer

  • Status changed from needs_work to needs_review
  • Work issues Use `cp -p` in `spkg-install` deleted

Replying to leif:

These two files are actually copied by spkg-install:

It is an upstream issue because the files have permission 0600 in the source tree, so they are copied to permission 0600 (both with and without -p).

The installation of the headers as executable files is an upstream problem. I have not really pinned it down, but the Singular install scripts are full of chmods.

Anyway, I have a new spkg up at http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-1-4.p11.spkg using cp -p for the help files and removing an existing chmod hack. I also made some file non-executable.

I do not want to make further changes here which are unrelated to the ticket. I agree with some of the points you make, but not for this ticket.

comment:15 follow-up: Changed 10 years ago by SimonKing

Hi Jeroen and Leif,

as far as I know, only a small change would be needed in the Singular spkg in order to fix #11645. Could that perhaps be included into your singular-3-1-1-4.p11.spkg as well?

comment:16 in reply to: ↑ 15 ; follow-up: Changed 10 years ago by jdemeyer

Replying to SimonKing:

as far as I know, only a small change would be needed in the Singular spkg in order to fix #11645. Could that perhaps be included into your singular-3-1-1-4.p11.spkg as well?

I would yes but only if it does not further delay the 4.7.1 release. I see no patch for #11645, do you have one?

comment:17 in reply to: ↑ 16 ; follow-ups: Changed 10 years ago by leif

Replying to jdemeyer:

Replying to SimonKing:

as far as I know, only a small change would be needed in the Singular spkg in order to fix #11645. Could that perhaps be included into your singular-3-1-1-4.p11.spkg as well?

I would yes but only if it does not further delay the 4.7.1 release. I see no patch for #11645, do you have one?

I've posted the patch upstream ("plain" context diff):

http://www.singular.uni-kl.de:8002/trac/raw-attachment/ticket/352/singular-trac_352-install_gftables_one_by_one.patch

comment:19 in reply to: ↑ 17 ; follow-up: Changed 10 years ago by leif

Replying to leif:

Replying to jdemeyer:

Replying to SimonKing:

as far as I know, only a small change would be needed in the Singular spkg in order to fix #11645. Could that perhaps be included into your singular-3-1-1-4.p11.spkg as well?

I would yes but only if it does not further delay the 4.7.1 release. I see no patch for #11645, do you have one?

I've posted the patch upstream ("plain" context diff)...

However, I can also quickly provide a new Singular spkg based on Karl-Dieter's (p12 then), based on yours, meanwhile p11. :)

Note also http://trac.sagemath.org/sage_trac/ticket/11645#comment:16; we already patch src/Singular/Makefile.in.

comment:20 in reply to: ↑ 19 Changed 10 years ago by leif

  • Status changed from needs_review to positive_review

Replying to leif:

Replying to leif:

I've posted the patch upstream ("plain" context diff)...

Note also http://trac.sagemath.org/sage_trac/ticket/11645#comment:16; we already patch src/Singular/Makefile.in.

FWIW, I've attached replacement files for patches/Singular-Makefile.in and patches/Singular-Makefile.in.patch to #11645, i.e. a cumulative patch to src/Singular/Makefile.in and the corresponding pre-patched file (which currently gets copied over into the upstream source tree).

Otherwise positive review for the current p11, as it now fixes the permissions issue (i.e., files not readable by "others"), though I think we should fix this in a more general way. (We'll have to check and probably change the upstream sources again on any upstream upgrade.)

Revert the status to "needs review" in case you want to fix #11645 here, too (by simply replacing the two files in patches/ as mentioned above, and of course updating SPKG.txt.)

comment:21 follow-up: Changed 10 years ago by kcrisman

  • Status changed from positive_review to needs_info

I hate to spoil this party, but where was the p10? Even SPKG.txt doesn't mention one.

Not sure if this means 'needs work', but at least 'needs info'!

comment:22 in reply to: ↑ 21 Changed 10 years ago by jdemeyer

Replying to kcrisman:

I hate to spoil this party, but where was the p10?

Sorry, I deleted it (before realizing that somebody might still be interested in it).

comment:23 Changed 10 years ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:24 follow-up: Changed 10 years ago by jdemeyer

  • Status changed from needs_review to positive_review

I believe kcrisman's comment should not affect the positive review...

comment:25 Changed 10 years ago by jdemeyer

I have posted a p12 at ticket #11645.

comment:26 in reply to: ↑ 24 Changed 10 years ago by kcrisman

Replying to jdemeyer:

I believe kcrisman's comment should not affect the positive review...

Okay, then I'll try to make a p13 for #11550. Please someone just give #11645 a positive review and not 'needs work', it is much more tedious for me to update spkgs than for the more technically astute, I'm afraid :(

comment:27 Changed 10 years ago by kcrisman

Followup p13 (based on #11645) at #11550.

comment:28 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.1.rc2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.