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

Python script of version_data #410

Merged
merged 8 commits into from Feb 20, 2018
Merged

Python script of version_data #410

merged 8 commits into from Feb 20, 2018

Conversation

gsmalik
Copy link
Contributor

@gsmalik gsmalik commented Feb 15, 2018

Fixes #405

Copy link
Member

@mithro mithro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for sending this pull request! It looks pretty good with some minor Python style stuff which needs fixing.

Could you also add a "Fixes #405" in the subject of the pull request?

@@ -0,0 +1,117 @@
import subprocess
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use Python 3, not Python 2.

Please make the script executable and have a correct hash bang at the top.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file still isn't Python 3.

import filecmp
import shutil

commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use PEP8 formatting for this file.

print branch[:-1]

describe = subprocess.Popen(['git', 'describe', '--dirty'], stdout=subprocess.PIPE)
describe,gar = describe.communicate();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stray semicolon on the end of this line.

import shutil

commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE)
commit,gar = commit.communicate()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gar?


temp_h = tempfile.NamedTemporaryFile( suffix='.h',delete='true')
print "Showing temp file .h"
print temp_h.name
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use triple quoted strings for a long string like this.

import filecmp
import shutil

commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should probably be using subprocess.check_call or subprocess.check_output calls instead of subprocess.Popen here.

import filecmp
import shutil

commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing comma / space between '-n' and 1.

status = subprocess.Popen(['git', 'status', '--short'], stdout=subprocess.PIPE)
status,gar = status.communicate();

platform = os.environ['PLATFORM']
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens when these values are not set in your environment?

@gsmalik gsmalik changed the title #405 Python script of version_data "Fixes #405" : Python script of version_data Feb 15, 2018
@mithro mithro changed the title "Fixes #405" : Python script of version_data Python script of version_data Feb 16, 2018
Copy link
Member

@mithro mithro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost there now! Just a few small style changes.


commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1'])
print("Showing commit variable")
print(commit[1:-2])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These print statements would probably be better like this,

print("Showing commit variable:", commit[1:-2])

import filecmp
import shutil

commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1'])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With Python 3 you want to explicitly decode the output from iostreams (like from subprocess).

stdout = subprocess.check_output(...).decode('utf-8')

length = status.count(b'\n')
print(status)

print("Showing uplatform")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uplatform?

temp_c.write(b"""\

#ifndef PLATFORM_%s
#error \"Version mismatch - PLATFORM_%s not defined!\"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to escape the quotes inside triple quote.

You should also look at the string.format here rather than string interpolation. I think it'll reduce the amount of duplication.

target.upper().encode(), target.upper().encode(), target.encode(),
commit[1:-2], branch[:-1], describe[:-1]))

for x in range(0, length):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for line in status.splitlines():
    ....

@@ -0,0 +1,99 @@
#!/usr/bin/env python

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a docstring for this file?

platform = os.environ['PLATFORM']
print(platform.upper())
else:
platform = ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should probably raise an error here with a useful message.

temp_h.seek(0)


temp_c = tempfile.NamedTemporaryFile(suffix='.c', delete='true')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

temp_c = tempfile.NamedTemporaryFile(suffix='.c', delete=True)

Copy link
Member

@mithro mithro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @gsmalik,

Code is looking really good now! Just a few very minor changes and it will be perfect.

Thank you for doing this!

Tim '@mithro' Ansell

import filecmp
import shutil

commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1'])\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A better way to wrap this line would be as follows;

branch = subprocess.check_output(
    ['git', 'symbolic-ref', '--short', 'HEAD']).decode('utf-8')

Generally you should never use the \ in Python.

platform = os.environ['PLATFORM']
else:
platform = ""
print("PLATFORM NOT SET")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please die with an error explaining something like

    sys.stderr.write("""\
PLATFORM environment variable is not set. 

This script should only be run as part of the LiteX Build Environment. 

Try `source ./scripts/enter-env.sh`"
""")
    sys.exit(1)


temp_c.write(string.format(platform.upper(), platform.upper(), platform,
target.upper(), target.upper(), target, commit[1:-2],
branch[:-1], describe[:-1]))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry I wasn't clear here! I mean to use "named formatting arguments". Please see this stackoverflow post -> https://stackoverflow.com/a/2452382 -- for example;

>>> "Hello {name}, your my {adjective} friend! Do you want to play a game {name}?".format(
...    name="gsmalik", adjective="best")
"Hello gsmalik, your my best friend! Do you want to play a game gsmalik?"
>>>

target = ""
print("TARGET NOT SET")

temp_h = tempfile.NamedTemporaryFile(suffix='.h', delete=True, mode='w+t')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small possible optimization here. As we are now in Python rather then shell, we probably don't need to use an actually on disk file here (IE A tempfile.NamedTemporaryFile object!). Instead we could just use a StringIO object, which is basically a mutable string that provides a file like interface. See example below;

Creating a file object using open();

f = open("myfile.txt", "w+t", encoding="utf-8")
f.write("some text data")
f.seek(0)
assert f.read() == "some text data"

Using a StringIO object for an in-memory objects which looks like a file;

f = io.StringIO()
f.write("some text data")
assert f.getvalue() == "some text data"

temp_c.write(' " --\\r\\n";')
temp_c.seek(0)


Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: One too many new lines here.

if not (filecmp.cmp(temp_c.name, 'version_data.c')):
print("Updating version_data.c")
os.remove('version_data.c')
shutil.copyfile(temp_c.name, 'version_data.c')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would add a new line between the shutil.copyfile(temp_c.name, 'version_data.c') and the following line.

@mithro
Copy link
Member

mithro commented Feb 20, 2018

Just one very minor change and then I can merge!

#!/usr/bin/env python
"""
This file embeds in the firmware information like the git commit
number,git branch information and git status of the build tree.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing the space between number,git

@mithro
Copy link
Member

mithro commented Feb 20, 2018

LGTM!

@mithro mithro merged commit a112978 into timvideos:master Feb 20, 2018
@mithro
Copy link
Member

mithro commented Feb 20, 2018

Merged! Thanks for contributing!

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

Successfully merging this pull request may close these issues.

None yet

2 participants