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

Conversation

Projects
None yet
2 participants
@gsmalik
Copy link
Contributor

gsmalik commented Feb 15, 2018

Fixes #405

@mithro
Copy link
Member

mithro left a comment

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

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

Please use Python 3, not Python 2.

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

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

This file still isn't Python 3.

import filecmp
import shutil

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

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

Please use PEP8 formatting for this file.

print branch[:-1]

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

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

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()

This comment has been minimized.

@mithro

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

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

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)

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

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)

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

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

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

platform = os.environ['PLATFORM']

This comment has been minimized.

@mithro

mithro Feb 15, 2018

Member

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

@mithro
Copy link
Member

mithro left a comment

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])

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

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'])

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

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")

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

uplatform?

temp_c.write(b"""\
#ifndef PLATFORM_%s
#error \"Version mismatch - PLATFORM_%s not defined!\"

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

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):

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member
for line in status.splitlines():
    ....
@@ -0,0 +1,99 @@
#!/usr/bin/env python

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

Can you add a docstring for this file?

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

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member

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')

This comment has been minimized.

@mithro

mithro Feb 16, 2018

Member
temp_c = tempfile.NamedTemporaryFile(suffix='.c', delete=True)
@mithro
Copy link
Member

mithro left a comment

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'])\

This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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")

This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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]))

This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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')

This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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)


This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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')

This comment has been minimized.

@mithro

mithro Feb 17, 2018

Member

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

@mithro

This comment has been minimized.

Copy link
Member

mithro commented Feb 20, 2018

Just one very minor change and then I can merge!

@mithro

mithro approved these changes Feb 20, 2018

#!/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.

This comment has been minimized.

@mithro

mithro Feb 20, 2018

Member

Still missing the space between number,git

@mithro

This comment has been minimized.

Copy link
Member

mithro commented Feb 20, 2018

LGTM!

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

4 checks passed

Codacy/PR Quality Review Good work! A positive pull request.
Details
WIP ready for review
Details
code-quality/landscape Landscape has completed the code quality check
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mithro

This comment has been minimized.

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