On Jun 7, 2008, at 12:31 PM, nathan binkert wrote:
> Couple of comments.
>
>
>> +# Generate hginfo.cc
>> +# Anything we pass as a source gets setup as a dependence so
>> rather than
>> +# passing the SConscript/hg dir/etc we just squirrel away the
>> SConstruct
>> +# directory in the environment and retrieve it later. This seems to
>> +# be the only reliable way to get the information if we're
>> building in
>> +# a directory outside of the m5 directory
>> +env['SConstructDir'] = str(SCons.Node.FS.default_fs.SConstruct_dir)
>> +env.Command('base/hginfo.cc', None, generate.hgInfo)
>> +
> Don't use your this environment variable hack. Instead, pass it as a
> source, but wrap it with Value(). Value basically stores only strings
> and if the string changes, causes the dependency to be regenerated.
> Make sense? You'll see me use it all over src/Sconscript.
Fine
>
>
>> + # Abuse the SCons dependence code to make the generation
>> + # of hginfo.cc dependend on all the other cc files and the
>> + # compiling of hginfo.cc dependent on all the objects
>> + # but hginfo.o
>> + hg_obj = env.Object('base/hginfo.cc')
>> + env.Depends('base/hginfo.cc', sources)
>> env.Depends(date_obj, objs)
>> - objs.append(date_obj)
>> + env.Depends(hg_obj, objs)
>> + objs.extend([date_obj,hg_obj])
>> return objs
> I don't quite know that this qualifies as abuse, but it works :)
Ok
>
>> + def hgInfo(self, target, source, env):
>> + try:
> This try block feels way too big and seems like it could hide some
> lurking Exceptions. Can you create a sub function that generates the
> file with the two parameters, and call it with unknown? e.g.
>
> try:
> import mercurial...
> except ImportError:
> generateFile("unknown", "unknown")
> return
>
> This would also force you to have the generation code in only one
> place. That way, if we ever add variables or something, we won't
> accidentally have two different versions.
fine
>
>
>> + # The SConscript squirrels away the SConstructDir
>> variable in the
>> + # env for us. We can't pass it as a source parameter
>> because that
>> + # would setup a depedence between everything in the
>> directory and
>> + # above and this file.
>> +
>> + scons_dir = env['SConstructDir']
> Again, this hack is not necessary.
removed
>
>> + import mercurial.hg, mercurial.ui, mercurial.util,
>> mercurial.node
>> + if not exists(scons_dir) or not isdir(scons_dir) or \
>> + not exists(join(scons_dir, ".hg")):
>> + raise ValueError
> Seems like you should check for the existence of .hg before you try to
> import mercurial.
>
>
>> + repo = mercurial.hg.repository(mercurial.ui.ui(),
>> scons_dir)
>> + rev = mercurial.node.nullrev + repo.changelog.count()
>> + changenode = repo.changelog.node(rev)
>> + changes = repo.changelog.read(changenode)
>> + date = mercurial.util.datestr(changes[2])
>> +
>> + hg_stats = file(str(target[0]), 'w')
>> + print >>hg_stats, 'const char *hgRev = "%s:%s";' %
>> (rev, mercurial.node.hex(changenode))
>> + print >>hg_stats, 'const char *hgDate = "%s";' % date
>> + hg_stats.close()
>> + mercurial.demandimport.disable()
>> + except ImportError:
>> + pass
>> + except:
>> + hg_stats = file(str(target[0]), 'w')
>> + print >>hg_stats, 'const char *hgRev = "Unknown";'
>> + print >>hg_stats, 'const char *hgDate = "Unknown";'
>> + hg_stats.close()
>> + mercurial.demandimport.disable()
> Why do you do this at the end? I'm not clear on the demandimport
> stuff, but it seems that you may as well just do it as soon as you've
> imported mercurial.
Because it needs to happen, any mercurial function can cause another
module to be imported.
>
>> +
>> +
>> +
>> +
> I object to random added whitespace.
>
>> diff --git a/src/python/m5/main.py b/src/python/m5/main.py
>> --- a/src/python/m5/main.py
>> +++ b/src/python/m5/main.py
>> @@ -269,6 +269,10 @@ def main():
>> print "M5 compiled %s" % internal.core.cvar.compileDate;
>> print "M5 started %s" % datetime.datetime.now().ctime()
>> print "M5 executing on %s" % socket.gethostname()
>> +
>> + print "M5 revision %s" % internal.core.cvar.hgRev
>> + print "M5 commit date %s" % internal.core.cvar.hgDate
>> +
>> print "command line:",
>> for argv in sys.argv:
>> print argv,
>> diff --git a/src/python/swig/core.i b/src/python/swig/core.i
>> --- a/src/python/swig/core.i
>> +++ b/src/python/swig/core.i
>> @@ -39,6 +39,8 @@
>> #include "sim/startup.hh"
>>
>> extern const char *compileDate;
>> +extern const char *hgRev;
>> +extern const char *hgDate;
>> %}
>>
>> %include "stdint.i"
>> @@ -51,6 +53,8 @@ void doExitCleanup();
>> void doExitCleanup();
>>
>> char *compileDate;
>> +char *hgRev;
>> +char *hgDate;
>>
>> void setClockFrequency(Tick ticksPerSecond);
>
> Seems all good to me.
>
>
> I like this idea. Are there any other variables that would be nice to
> add? Versions of gcc and libraries and such?
>
> Nate
> _______________________________________________
> m5-dev mailing list
> [email protected]
> http://m5sim.org/mailman/listinfo/m5-dev
>
_______________________________________________
m5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/m5-dev
