Add short git revision to 'topaz -v'. #514

parkr · 2013-03-16T15:29:06Z

Fix for #482. When using topaz, it's easier to submit bugs we encounter when we know how the git revision of topaz.

meatballhat · 2013-03-16T15:52:18Z

topaz/main.py

@@ -186,9 +186,10 @@ def _entry_point(space, argv):
        system, _, _, _, cpu = os.uname()
    platform = "%s-%s" % (cpu, system.lower())
    engine = "topaz"
+    git_hash = os.popen("git rev-parse --short HEAD", "r").readline()


This won't achieve what you're describing as the popen call is during VM startup rather than being "frozen" at translation time. Perhaps if you move the call to the module namespace and assign it to a const a la (untested):

GIT_REV = os.popen("git rev-parse --short HEAD", "r").readline() # ... then, in `_entry_point` description = "%s (ruby-%sp%d) [%s] (git rev %s)" % (engine, version, patchlevel, platform, GIT_REV)

meatballhat · 2013-03-16T15:54:28Z

Also, please modify the associated tests (see Travis failure). Thank you!

parkr · 2013-03-16T16:52:10Z

Not sure why I was getting the "ValueError: too many values to unpack" errors. I wasn't adding a newline anywhere and the call is to splitlines(), which splits along the \n I would guess.

parkr · 2013-03-16T16:52:52Z

Figured it out. Will push a fix :)

parkr · 2013-03-16T16:55:19Z

Also, I can't seem to run the tests locally. Every test gives me: E ImportError: No module named rpython.

I ran the following to install/run tests:

/usr/local/share/pypy/pip install -r requirements.txt
/usr/local/share/pypy/py.test

meatballhat · 2013-03-16T17:02:27Z

You need to have a clone of pypy on your PYTHONPATH *. It's somewhere in the docs... I think... 😕

until rpython gets fully extracted...

parkr · 2013-03-16T17:03:40Z

A clone of the pypy repo? That's in the docs (in the README.md) but isn't that explicit. Seemed that having the pypy executable satisfied that requirement.

meatballhat · 2013-03-16T17:04:30Z

tests/test_main.py

@@ -275,7 +277,7 @@ def test_ruby_description(self, space, tmpdir, capfd):
        self.run(space, tmpdir, "puts RUBY_DESCRIPTION")
        out1, err1 = capfd.readouterr()
        self.run(space, tmpdir, """
-        puts "#{RUBY_ENGINE} (ruby-#{RUBY_VERSION}p#{RUBY_PATCHLEVEL}) [#{RUBY_PLATFORM}]"
+        puts "#{RUBY_ENGINE} (ruby-#{RUBY_VERSION}p#{RUBY_PATCHLEVEL}) [#{RUBY_PLATFORM}] (git rev #{GIT_REV})"


Unless you've explicitly exported GIT_REV into the object space, this won't resolve. I'm not sure it should even be exported, but if so it should probably have a TOPAZ_ prefix at the very least. I'm strongly deferring to @alex and @timfel :-)

I can read, I swear. I see you're adding a GIT_REV const, so yay! As for the right name, I still defer to @alex & @timfel.

I agree - I think the TOPAZ_ prefix is better.

parkr · 2013-03-16T17:06:31Z

The colors produced by the translate test are MAD trippy. And super fun to watch.

meatballhat · 2013-03-16T17:07:33Z

Fractals rule 🤘

parkr · 2013-03-16T17:44:06Z

Kind of sucks that the tests take 15 minutes to run.

timfel · 2013-03-16T17:57:50Z

Translation is what takes so long. You don't need to run that locally, just run pytest and any specs that you're interested in untranslated. Also, you don't need a pypy executable at all, you only need the source tree.

timfel · 2013-03-16T18:00:31Z

GIT_REV should be called RUBY_REVISION

meatballhat · 2013-03-16T18:27:57Z

👍 to RUBY_REVISION (future-proofing win!)

alex · 2013-03-16T21:13:35Z

topaz/main.py

@@ -39,7 +39,7 @@
    ""
 ])
 COPYRIGHT = "topaz - Copyright (c) Alex Gaynor and individual contributors\n"
-
+RUBY_REVISION = os.popen("git rev-parse --short HEAD", "r").readline().rstrip()


This should be named TOPAZ_REVISION or something.

It's undergone quite a long series of name changes. git_hash ~> GIT_REV ~> TOPAZ_GIT_REV ~> RUBY_REVISION. Is TOPAZ_REVISION your final answer? :jeopardy:

Mri, Maglev, Rbx, and Jruby all define RUBY_RREVISON

Oh, then ignore me :)

Can you rewrite the line as subprocess.check_output(["git", "rev-parse", "--short", "HEAD"]).rstrip()

parkr · 2013-03-16T22:28:49Z

@alex Donezo

parkr · 2013-03-17T14:06:28Z

@alex @timfel @meatballhat Reverted the name change, using subprocess and squashed into 1 commit.

meatballhat · 2013-03-17T16:11:13Z

Looks like you're missing your subprocess import in topaz/main.py. Even if you don't yet have a working py.test setup, you can test this like:

python -m topaz -v -e "puts RUBY_REVISION"

Add short git revision to 'topaz -v'.

parkr · 2013-03-17T16:48:52Z

Thank the Lord! That was quite a back-and-forth. Please make sure you close #482 as well!

meatballhat · 2013-03-17T17:17:55Z

@parkr and you thought it was over! 😸

MRI does the revision before the architecture-platform string:

~$ ruby -v
ruby 1.9.3p392 (2013-02-22 revision 39386) [x86_64-darwin11.4.2]

dunno how much we care about encouraging bad behavior around parsing that string, though...

parkr · 2013-03-17T18:35:47Z

so you'd rather go with MRI's convention?

meatballhat · 2013-03-17T19:45:34Z

It was mostly just an observation. I leave it to @alex and @timfel to make the call. To your point, this issue/PR has felt a lot like Topaz/PyPy hazing :-P

parkr · 2013-03-17T19:47:01Z

Haha it's just so young that conventions are hard to infer :)

meatballhat · 2013-03-17T19:48:22Z

👍 it's a land of opportunity!

alex · 2013-03-17T19:53:51Z

I'll let @timfel have teh final say, copying the style makes a bit of sense to me though.

timfel · 2013-03-17T21:47:01Z

I agree with @alex, lets copy MRI where we can

chrblabla · 2013-03-17T23:00:02Z

I think this just broke RVM installation as it now complains that "this" is not a git repository:

fatal: Not a git repository (or any of the parent directories): .git
Traceback (most recent call last):
  File "/home/chris/.rvm/src/pypy/rpython/translator/goal/translate.py", line 333, in <module>
    main()
  File "/home/chris/.rvm/src/pypy/rpython/translator/goal/translate.py", line 218, in main
    targetspec_dic, translateconfig, config, args = parse_options_and_load_target()
  File "/home/chris/.rvm/src/pypy/rpython/translator/goal/translate.py", line 154, in parse_options_and_load_target
    targetspec_dic = load_target(targetspec)
  File "/home/chris/.rvm/src/pypy/rpython/translator/goal/translate.py", line 100, in load_target
    mod = __import__(specname)
  File "/home/chris/.rvm/src/topaz-head/targettopaz.py", line 3, in <module>
    from topaz.main import entry_point
  File "/home/chris/.rvm/src/topaz-head/topaz/main.py", line 43, in <module>
    RUBY_REVISION = subprocess.check_output(["git", "rev-parse", "--short", "HEAD"]).rstrip()
  File "/usr/lib/python2.7/subprocess.py", line 544, in check_output
    raise CalledProcessError(retcode, cmd, output=output)
subprocess.CalledProcessError: Command '['git', 'rev-parse', '--short', 'HEAD']' returned non-zero exit status 128

parkr · 2013-03-17T23:44:45Z

is the revision cached at translation time?

meatballhat · 2013-03-18T03:13:13Z

Looks like RVM is running the translation (makes sense) so presumably we'll have to either fake out RUBY_REVISION since git isn't reliably available or do Something Else © such as fetching the git rev from a file deposited during some mythical tarball-building step. I'm not familiar with the Topaz RVM "recipe" (or whatever they call it) -- is it downloading a tarball directly from github master, perhaps?

timfel · 2013-03-18T07:16:15Z

Rvm downloads a binary tarball ball unless you request a HEAD install. The head install does clone the proper git repository, but it may not start the translation from the topaz directory. From the looks of the suvprocess invocation you have to be in the topaz directory to run it, correct? Would a chdir to os.path.dirname(__file__) suffice around the subprocess execution?

parkr · 2013-03-18T10:43:01Z

You'll have to be in the topaz directory, assuming it contains the .git folder.

meatballhat reviewed Mar 16, 2013
View reviewed changes

alex reviewed Mar 16, 2013
View reviewed changes

parkr added 2 commits March 17, 2013 15:08

Add short git revision to 'topaz -v'.

39679ba

Bring subprocess into test file.

ef9d203

Import subprocess into topaz/main.py

d3c8cf7

alex added a commit that referenced this pull request Mar 17, 2013

Merge pull request #514 from parkr/topaz-v-git-hash

2405d6d

Add short git revision to 'topaz -v'.

alex merged commit 2405d6d into topazproject:master Mar 17, 2013

parkr deleted the topaz-v-git-hash branch March 17, 2013 16:48

parkr mentioned this pull request Mar 17, 2013

topaz -v should output git hash to track development versions/builds more easily #482

Closed

meatballhat mentioned this pull request Mar 18, 2013

RUBY_REVISION breaks rvm #523

Open

chrblabla mentioned this pull request Mar 19, 2013

git call to get current revision for RUBY_REVISION #531

Closed

meatballhat mentioned this pull request Mar 21, 2013

Moving the RUBY_REVISION part of version string to be more MRI-ish #537

Merged

Add short git revision to 'topaz -v'. #514

Add short git revision to 'topaz -v'. #514

Conversation

parkr commented Mar 16, 2013

Choose a reason for hiding this comment

meatballhat commented Mar 16, 2013

parkr commented Mar 16, 2013

parkr commented Mar 16, 2013

parkr commented Mar 16, 2013

meatballhat commented Mar 16, 2013

parkr commented Mar 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Mar 16, 2013

meatballhat commented Mar 16, 2013

parkr commented Mar 16, 2013

timfel commented Mar 16, 2013

timfel commented Mar 16, 2013

meatballhat commented Mar 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkr commented Mar 16, 2013

parkr commented Mar 17, 2013

meatballhat commented Mar 17, 2013

parkr commented Mar 17, 2013

meatballhat commented Mar 17, 2013

parkr commented Mar 17, 2013

meatballhat commented Mar 17, 2013

parkr commented Mar 17, 2013

meatballhat commented Mar 17, 2013

alex commented Mar 17, 2013

timfel commented Mar 17, 2013

chrblabla commented Mar 17, 2013

parkr commented Mar 17, 2013

meatballhat commented Mar 18, 2013

timfel commented Mar 18, 2013

parkr commented Mar 18, 2013