Opened 3 years ago

Closed 3 years ago

#28596 closed defect (fixed)

Fix jmol permissions

Reported by: jhpalmieri Owned by:
Priority: major Milestone: sage-9.0
Component: packages: standard Keywords:
Cc: jdemeyer, embray, novoselt Merged in:
Authors: Erik Bray Reviewers: John Palmieri
Report Upstream: N/A Work issues:
Branch: 87c694f (Commits, GitHub, GitLab) Commit: 87c694f059e839a4bb12db80037f3c4398b56c2b
Dependencies: Stopgaps:

Status badges

Description (last modified by jhpalmieri)

This is a followup to #25026: the permissions of SAGE_ROOT/local/share/jmol/ are wrong:

  drwxr-xr-x 24 palmieri staff  768 Sep 12 00:11 info
  drwx------ 21 palmieri staff  672 Oct 12 10:28 jmol
  drwxr-xr-x 80 palmieri staff 2560 Oct 12 10:28 jsmol
  drwxr-xr-x  4 palmieri staff  128 Sep 12 00:11 jupyter

Change History (16)

comment:1 Changed 3 years ago by jhpalmieri

  • Description modified (diff)

comment:2 Changed 3 years ago by jhpalmieri

  • Cc jdemeyer embray novoselt added

I can see two solutions: change jmol's spkg-install script so it sets the right permissions, or set the permissions differently for all packages. Here is a branch which does the second of these: after unpacking the tarball into src, run chmod +rx src. Is this a bad idea for some reason?

comment:3 Changed 3 years ago by jhpalmieri

  • Branch set to u/jhpalmieri/jmol-permissions

comment:4 Changed 3 years ago by jhpalmieri

  • Commit set to 82e736b7c1f5f93042d61d34149150ab7d9cec2b
  • Status changed from new to needs_review

New commits:

82e736btrac 28596: change permissions on src after unpacking each package's tarball.

comment:5 Changed 3 years ago by jhpalmieri

I want to do this for all packages to avoid similar situations in the future.

comment:6 follow-ups: Changed 3 years ago by chapoton

This does not seem to fix the doc building issue.

comment:7 in reply to: ↑ 6 Changed 3 years ago by embray

Replying to chapoton:

This does not seem to fix the doc building issue.

I don't know what doc building issue you're referencing.

comment:8 Changed 3 years ago by chapoton

Here is the docbuild log where the problem appears:

https://patchbot.sagemath.org/log/0/Ubuntu/16.04/x86_64/4.4.0-166-generic/atlas/2019-11-27%2010:02:21?plugin=docbuild

This prevents me from using this machine as a patchbot.

comment:9 Changed 3 years ago by embray

  • Status changed from needs_review to needs_work

This behavior originates from #24567, and would be different if we'd gone with Jeroen's suggestion of a less restricted mask on the files than I used.

I wrote at the time:

this should have no effect on the permissions applied when the files are installed, but I guess it depends in part on the installation mechanism

Which, incidentally was true at the time. But the part of "depends on the installation mechanism" was also true, and the installation mechanism did change after that, in such a way that this did have an effect on the installed files.

So I think the better option here might be to try Jeroen's original suggestion of a less restrictive permission mask on the source tarball's extraction location.

comment:10 in reply to: ↑ 6 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • Branch changed from u/jhpalmieri/jmol-permissions to u/embray/ticket-28596
  • Commit changed from 82e736b7c1f5f93042d61d34149150ab7d9cec2b to 87c694f059e839a4bb12db80037f3c4398b56c2b

This approach fixes the issue closer to the source. However, it would be good to test a full rebuild. I don't know for sure if there aren't some pathological packages that can break because of this (and if so those packages should probably be fixed...).


New commits:

df58d4bTrac #28596: Apply less restricted permission mask to extracted source
87c694fAdd an spkg-legacy-uninstall for jmol.

comment:11 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:12 Changed 3 years ago by jhpalmieri

All tests pass for me on OS X, with a build from scratch, and the permissions look okay.

comment:13 Changed 3 years ago by chapoton

then it should be ok, no ?

comment:14 Changed 3 years ago by jhpalmieri

  • Reviewers set to John Palmieri
  • Status changed from needs_review to positive_review

Okay with me.

comment:15 Changed 3 years ago by embray

I'm sure, if he were paying attention, Jeroen would be pleased to be vindicated :)

comment:16 Changed 3 years ago by vbraun

  • Branch changed from u/embray/ticket-28596 to 87c694f059e839a4bb12db80037f3c4398b56c2b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.