Opened 8 years ago

Closed 8 years ago

#16827 closed defect (fixed)

Use atomic_write in sage-preparse

Reported by: jdemeyer Owned by:
Priority: minor Milestone: sage-6.4
Component: scripts Keywords:
Cc: Merged in:
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 8601fdd (Commits, GitHub, GitLab) Commit: 8601fddda9fd3526b7c68c9e1d07485e70e759db
Dependencies: Stopgaps:

Status badges

Description

To avoid race conditions, the script sage-preparse should use atomic_write.

Change History (7)

comment:1 Changed 8 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/16827
  • Created changed from 08/14/14 11:50:12 to 08/14/14 11:50:12
  • Modified changed from 08/14/14 11:50:12 to 08/14/14 11:50:12

comment:2 Changed 8 years ago by jdemeyer

  • Commit set to 8601fddda9fd3526b7c68c9e1d07485e70e759db
  • Status changed from new to needs_review

New commits:

​8601fddUse atomic_write in sage-preparse

comment:3 Changed 8 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

comment:4 Changed 8 years ago by jhpalmieri

I know this isn't part of this ticket, but there is a long-standing problem with preparsing that should be easy to fix: if you happen to give a file the same name as a Python module (which is not hard to do; I've seen people use "new.sage" and "socket.sage" before), then the preparsed file can shadow the actual Python module. I think a patch like this ought to fix it:

  • src/bin/sage-preparse 

    diff --git a/src/bin/sage-preparse b/src/bin/sage-preparse
index e2d5e9c..dff9800 100755
    a b def do_preparse(f, files_before=[]): 
    8686        print "%s: Unknown file type %s"%(sys.argv[0], f)
    8787        sys.exit(1)
    8888
    89     fname = '%s.py'%f[:-5]
     89    fname = '%s_preparsed.py'%f[:-5]
    9090    if os.path.exists(fname):
    9191        if AUTOGEN_MSG not in open(fname).read():
    9292            print "Refusing to overwrite existing non-autogenerated file '%s'."%os.path.abspath(fname)

  • src/bin/sage-run 

    diff --git a/src/bin/sage-run b/src/bin/sage-run
index 3f2700b..638c770 100755
    a b if fn.startswith('-'): 
    2020if fn.endswith('.sage'):
    2121    if call(['sage-preparse', fn]) != 0:
    2222        sys.exit(1)
    23     os.execvp('sage-python', ['sage-python', fn[:-5] + '.py'] + opts)
     23    os.execvp('sage-python', ['sage-python', fn[:-5] + '_preparsed.py'] + opts)
    2424elif fn.endswith('.pyx') or fn.endswith('.spyx'):
    2525    os.execvp('sage-run-cython', ['sage-run-cython', fn] + opts)
    2626else:

Alternatively, there is another approach at #11821, which also might prevent the race conditions discussed here: don't write the preparsed output to a file, just use standard output.

comment:5 Changed 8 years ago by vbraun

John, can you move you comment to a new ticket? I'm going to merge and close this ticket now.

comment:6 Changed 8 years ago by jdemeyer

New ticket: #16955

comment:7 Changed 8 years ago by vbraun

  • Branch changed from u/jdemeyer/ticket/16827 to 8601fddda9fd3526b7c68c9e1d07485e70e759db
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.