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
Conversation
There was a problem hiding this 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?
firmware/version_data.py
Outdated
@@ -0,0 +1,117 @@ | |||
import subprocess |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
import filecmp | ||
import shutil | ||
|
||
commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE) |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
print branch[:-1] | ||
|
||
describe = subprocess.Popen(['git', 'describe', '--dirty'], stdout=subprocess.PIPE) | ||
describe,gar = describe.communicate(); |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
import shutil | ||
|
||
commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE) | ||
commit,gar = commit.communicate() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gar
?
firmware/version_data.py
Outdated
|
||
temp_h = tempfile.NamedTemporaryFile( suffix='.h',delete='true') | ||
print "Showing temp file .h" | ||
print temp_h.name |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
import filecmp | ||
import shutil | ||
|
||
commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE) |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
import filecmp | ||
import shutil | ||
|
||
commit = subprocess.Popen(['git', 'log', '--format="%H"', '-n''1'], stdout=subprocess.PIPE) |
There was a problem hiding this comment.
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
.
firmware/version_data.py
Outdated
status = subprocess.Popen(['git', 'status', '--short'], stdout=subprocess.PIPE) | ||
status,gar = status.communicate(); | ||
|
||
platform = os.environ['PLATFORM'] |
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
firmware/version_data.py
Outdated
|
||
commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1']) | ||
print("Showing commit variable") | ||
print(commit[1:-2]) |
There was a problem hiding this comment.
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])
firmware/version_data.py
Outdated
import filecmp | ||
import shutil | ||
|
||
commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1']) |
There was a problem hiding this comment.
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')
firmware/version_data.py
Outdated
length = status.count(b'\n') | ||
print(status) | ||
|
||
print("Showing uplatform") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uplatform?
firmware/version_data.py
Outdated
temp_c.write(b"""\ | ||
|
||
#ifndef PLATFORM_%s | ||
#error \"Version mismatch - PLATFORM_%s not defined!\" |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
target.upper().encode(), target.upper().encode(), target.encode(), | ||
commit[1:-2], branch[:-1], describe[:-1])) | ||
|
||
for x in range(0, length): |
There was a problem hiding this comment.
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():
....
firmware/version_data.py
Outdated
@@ -0,0 +1,99 @@ | |||
#!/usr/bin/env python | |||
|
There was a problem hiding this comment.
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?
firmware/version_data.py
Outdated
platform = os.environ['PLATFORM'] | ||
print(platform.upper()) | ||
else: | ||
platform = "" |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
temp_h.seek(0) | ||
|
||
|
||
temp_c = tempfile.NamedTemporaryFile(suffix='.c', delete='true') |
There was a problem hiding this comment.
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)
… prints.4. Better decoing to UTF-8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
firmware/version_data.py
Outdated
import filecmp | ||
import shutil | ||
|
||
commit = subprocess.check_output(['git', 'log', '--format="%H"', '-n', '1'])\ |
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
platform = os.environ['PLATFORM'] | ||
else: | ||
platform = "" | ||
print("PLATFORM NOT SET") |
There was a problem hiding this comment.
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)
firmware/version_data.py
Outdated
|
||
temp_c.write(string.format(platform.upper(), platform.upper(), platform, | ||
target.upper(), target.upper(), target, commit[1:-2], | ||
branch[:-1], describe[:-1])) |
There was a problem hiding this comment.
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?"
>>>
firmware/version_data.py
Outdated
target = "" | ||
print("TARGET NOT SET") | ||
|
||
temp_h = tempfile.NamedTemporaryFile(suffix='.h', delete=True, mode='w+t') |
There was a problem hiding this comment.
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"
firmware/version_data.py
Outdated
temp_c.write(' " --\\r\\n";') | ||
temp_c.seek(0) | ||
|
||
|
There was a problem hiding this comment.
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.
firmware/version_data.py
Outdated
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') |
There was a problem hiding this comment.
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.
Just one very minor change and then I can merge! |
firmware/version_data.py
Outdated
#!/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. |
There was a problem hiding this comment.
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
LGTM! |
Merged! Thanks for contributing! |
Fixes #405