Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

buffer.py: make files readonly only when possible #33256

Closed
kiwifb opened this issue Jan 31, 2022 · 11 comments
Closed

buffer.py: make files readonly only when possible #33256

kiwifb opened this issue Jan 31, 2022 · 11 comments

Comments

@kiwifb
Copy link
Member

kiwifb commented Jan 31, 2022

After #31306 we have doctest failure on sage-on-distros because files have been moved out of SAGE_EXTCODE. In sage/repl/rich_output/buffer.py files are made readonly with chmod so they are immutable during the process. Unfortunately chmod fails when trying to act on system installed files as a regular user. There is a safety for files in SAGE_EXTCODE, this is a useless complication and should be replaced by a try block.

CC: @mkoeppe

Component: distribution

Author: François Bissey

Branch/Commit: 2c3a144

Reviewer: Antonio Rojas

Issue created by migration from https://trac.sagemath.org/ticket/33256

@kiwifb kiwifb added this to the sage-9.6 milestone Jan 31, 2022
@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

comment:1

While we may want to avoid error on file that are not changeable by the user, just a try doesn't reproduce the original intent

        if filename.startswith(os.path.abspath(SAGE_EXTCODE)):
            # Do not change permissions on the sample rich output
            # files, as it will cause trouble when upgrading Sage

Should the original block be preserved but with SAGE_SRC instead of SAGE_EXTCODE?

@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

New commits:

2c3a144Make _chmod_readonly more robust. Extend sample protection to SAGE_SRC as samples have been moved out of SAGE_EXTCODE

@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

Branch: u/fbissey/chmod_readonly

@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

Author: François Bissey

@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

Commit: 2c3a144

@mkoeppe
Copy link
Contributor

mkoeppe commented Jan 31, 2022

comment:4

I don't know the details about this code there, but for the failures in #31306 I think the right fix is to change the doctests so that they don't attempt to make changes (including permissions) to the source tree.

@kiwifb
Copy link
Member Author

kiwifb commented Jan 31, 2022

comment:5

Replying to @mkoeppe:

I don't know the details about this code there, but for the failures in #31306 I think the right fix is to change the doctests so that they don't attempt to make changes (including permissions) to the source tree.

That's what I thought and why I switched the test to check for SAGE_SRC (it works as expected here).

What about the other part of the branch which basically is about not failing if you cannot change the permissions outside of the source tree? The code wants to make sure the file that it is dealing with cannot be modified by changing its permission, but will fail when it cannot do so - in which case you probably cannot modify the file already.

@antonio-rojas
Copy link
Contributor

comment:7

Works fine here (also testing on installed package)

@antonio-rojas
Copy link
Contributor

Reviewer: Antonio Rojas

@kiwifb
Copy link
Member Author

kiwifb commented Feb 6, 2022

comment:8

Thanks!

@vbraun
Copy link
Member

vbraun commented Feb 16, 2022

Changed branch from u/fbissey/chmod_readonly to 2c3a144

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants