Opened 3 years ago

Closed 3 years ago

#23095 closed defect (fixed)

autogen/interpreters does NOT work

Reported by: jdemeyer Owned by:
Priority: blocker Milestone: sage-8.0
Component: build Keywords:
Cc: embray, jpflori Merged in:
Authors: Jeroen Demeyer Reviewers: Erik Bray
Report Upstream: N/A Work issues:
Branch: aaedeff (Commits) Commit: aaedeff71f36c5d55545d8594dbfee76b9ad1939
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

After making a minor change to src/sage_setup/autogen/interpreters/generator.py, the change did not take effect. I.e. the generated file src/sage/ext/interpreters/wrapper_rdf.pyx did not change.

In particular, the following should cause an error while building the interpreters, but it did not:

  • src/sage_setup/autogen/interpreters/generator.py

    diff --git a/src/sage_setup/autogen/interpreters/generator.py b/src/sage_setup/autogen/interpreters/generator.py
    index 3af99b2..5a3bfbb 100644
    a b class InterpreterGenerator(object): 
    316316            sage: print(buff.getvalue())
    317317            # Automatically generated by ...
    318318        """
     319        assert False
     320
    319321        s = self._spec
    320322        w = write
    321323        types = set()

The reason is that this only checks one source file (src/sage_setup/autogen/interpreters/__init__.py):

    # Although multiple files are generated by this function, since
    # they are all generated at once it suffices to make sure if just
    # one of the generated files is older than the generator sources
    src_file = __file__
    gen_file = os.path.join(dirname, '__init__.py')

    if os.path.exists(gen_file):
        if getmtime(gen_file) > getmtime(src_file) and not force:
            return

Change History (13)

comment:1 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:3 Changed 3 years ago by embray

I think at some earlier version of this (before it was merged) it did check all source files? I don't know when it changed.

comment:4 Changed 3 years ago by embray

I think there was one point where it was src_files but in one of the last rebases it got messed up.

comment:5 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/23095

comment:6 Changed 3 years ago by jdemeyer

  • Cc jpflori added
  • Commit set to 82f91f2ba83f2fd6a000976d719556252bf6440a
  • Status changed from new to needs_review

New commits:

82f91f2Rebuild interpreters if any source file changed

comment:7 Changed 3 years ago by embray

  • Reviewers set to Erik Bray
  • Status changed from needs_review to needs_work

I'm not sure why you're using OSError here. It doesn't really matter (actually I'd say it does matter--I think a real OSError should be allowed to rise here, or at least be handled separately), but it looks odd. If the point is just to catch some exception just a generic Exception would be fine. Looks good to me otherwise; sorry for any inconvenience this might have caused.

Last edited 3 years ago by embray (previous) (diff)

comment:8 Changed 3 years ago by jdemeyer

At first, I created a separate exception class for this (say class NeedToRebuild(Exception)). But then I realized that it would be good to catch OSError too since there are various system calls in that block of code which could potentially raise OSError. Since this is only in determining whether to rebuild, I figured that it shouldn't be a fatal error.

comment:9 Changed 3 years ago by embray

I get that--I guess it doesn't need to be a fatal error then, but at least some more explanation surrounding an actual OSError would be good (just like "checking if rebuild needed failed: <actual error>").

comment:10 Changed 3 years ago by git

  • Commit changed from 82f91f2ba83f2fd6a000976d719556252bf6440a to aaedeff71f36c5d55545d8594dbfee76b9ad1939

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

aaedeffRebuild interpreters if any source file changed

comment:11 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

I don't want to make this more complicated than it needs to be. This is reverting to my original idea.

comment:12 Changed 3 years ago by embray

  • Status changed from needs_review to positive_review

Okay, seems fine.

comment:13 Changed 3 years ago by vbraun

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