1116553 - Rewrite version_win.pl in Python

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[(not currently active) Ted Mielczarek]

It lives here:
http://dxr.mozilla.org/mozilla-central/source/config/version_win.pl

And it gets called from here:
http://dxr.mozilla.org/mozilla-central/source/config/version.mk

We should rewrite it in Python, it's pretty simple and it would get rid of one more Perl script. version.mk is the only place it's called, so any commandline arguments that it supports that aren't used there aren't necessary (whoever takes this doesn't have to do a full port of all supported options).

Fundamentally what this script does is pretty simple:
1) Parse some commandline options.
2) Read milestone.txt and the buildid file for some input data (version number and build id)
3) Read $srcdir/module.ver if it exists for some override values
4) Calculate a few trivial values based on the inputs from the commandline and the files in steps 2 and 3
5) Write to $objdir/module.rc:
a) A static header
b) The contents of the RCINCLUDE file from the commandline options (with a s/\@MOZ_APP_DISPLAYNAME\@/$displayname/)
c) A template string including some values from the inputs or the calculations in step 4

Comment 1

[talinpaul]

So i am a beginer as you might have guessed... I've started working on a peice of Python code...

Comment 2

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

You may wish to build on to of the patch in bug 739601; it already provides some of the code you'll need.

Comment 3

[talinpaul]

Attachment: Though it is not complete yet... This is what i propose...

Here is what i've worked on so far... Criticisms are welcome (Dont be too harsh on me...)

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8543759 [details]
Though it is not complete yet... This is what i propose...

This actually looks like a good start! I'm going to set the feedback flag on this so that I remember to give it a more thorough look, but I think you're soundly on the right track.

Do you have a local Firefox build environment set up? To contribute effectively you'll want to do that, so at the very least you can provide patches against our current source code. The documentation here should help:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

If you need more help you can join our IRC server at irc.mozilla.org and join the #introduction channel.

Thanks for contributing!

Comment 5

[(not currently active) Ted Mielczarek]

I'm going to assign this bug to you, which will let other people know you're actively working on it.

Comment 6

[talinpaul]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)
> I'm going to assign this bug to you, which will let other people know you're
> actively working on it.

Thanks for the feedback...

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8543759 [details]
Though it is not complete yet... This is what i propose...

This is a great start, thanks! I'm going to give you a lot of feedback here but don't panic! Most of it is pretty straightforward. Did you get a local copy of the Firefox repository to work with? Submitting patches that way will make everyone's life easier, and if you have a Windows machine you can actually build the source and test that your patch does what you want.

You'll want to put a license header at the top of this file, you can just copy one from another Python file in the source tree. Also: a minor note but we use single quotes (') for strings in all our new Python.

>import sys
>import os
>import argparse
>
>#1) Parse some commandline options.
>
>#Creates version resource file
>
>#Paramaters are passed on the command line:
>
>#Example: -MODNAME nsToolkitCompsModule -DEBUG=1
>
># DEBUG - Mozilla's global debug variable - tells if its debug version
># OFFICIAL - tells Mozilla is building a milestone or nightly
># MSTONE - tells which milestone is being built;
># OBJDIR - Holds the object directory;
># MODNAME - tells what the name of the module is like nsBMPModule
># DEPTH - Holds the path to the root obj dir
># TOPSRCDIR - Holds the path to the root mozilla dir
># SRCDIR - Holds module.ver and source
># BINARY - Holds the name of the binary file
># DISPNAME - Holds the display name of the built application
># APPVERSION - Holds the version string of the built application
># RCINCLUDE - Holds the name of the RC File to include or ""
># QUIET - Turns off output
>
>#Description and Comment come from module.ver
>#Bug 23560
>#http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/rc_7x2d.asp
>
>################################################################################
># ToDo: Code in progress
>################################################################################
>def getDaysFromBuild():
>    # Add Later...
>    return days_from_build
>def getOfficialMilestone(milestone_file):
>    with milestone_file as file:
>      for line in file:
>          if bool(re.match("^\d",line)):
>             milestone = line
>             return milestone
>
>module = None
>
>parser = argparse.ArgumentParser(description='Argument parser...')
># Adding arguments.
>parser.add_argument('--quiet',      metavar='QUIET',        help='Turns off output')
>parser.add_argument('--debug',      metavar='DEBUG',        help='Mozilla\'s global debug variable - tells if its debug version')
>parser.add_argument('--official',   metavar='OFFICIAL',     help='tells Mozilla is building a milestone or nightly')

>parser.add_argument('--mstone',     metavar='MSTONE',       help='tells which milestone is being built')
>parser.add_argument('--modname',    metavar='MODNAME',      help='tells what the name of the module is like nsBMPModule')

We don't seem to use these two arguments in version.mk, so you can leave them out.

I noticed two things:
1) The original script uses UPPERCASE for its options.
2) The original script passes arguments with a single leading dash, like -QUIET.

I'm not even sure you can support #2 with argparse, so it's probably not worthwhile. This means you'll have to change version.mk as well:
https://hg.mozilla.org/mozilla-central/annotate/29b05d283b00/config/version.mk

So that things like '-QUIET 1' become '--quiet 1'. Since you're making that change you could also change any of these arguments that take a 1 as the option value to just be action='store_true' in the parser.add_argument call, so that you could simply pass '--quiet'.


># Parsing the args...
>args = vars(parser.parse_args())
>
>#Adding default arguments...

You can set these defaults up above in the calls to add_argument, it takes a 'default=' keyword argument.

>
>#I dont understand what these are.... So {{Voodoo}} Do not touch unless you understand...
>mfversion = "Personal"
>mpversion = "Personal"

These don't seem to ever be used in the original script, they're always overridden by reading milestone.txt here:
https://hg.mozilla.org/mozilla-central/annotate/29b05d283b00/config/version_win.pl#l192

>comment = None
>description = None
>fileflags = ["0"]
>
>#still not used
>bufferstr = ""
>
>#2) Read milestone.txt and the buildid file for some input data (version number and build id)
># Figuring out what to do with this....
># Figured out that we are supposed to read some values and voodoo dance with it!!!
>MILESTONE_FILE = open( os.path.join( args['topsrcdir'] + "/config/milestone.txt"), 'r')

Lucky for you in the interim here Ms2ger already wrote this in Python, so you can just do:

from mozbuild import milestone
m = milestone.get_official_milestone(os.path.join(args['topsrcdir'], 'config/milestone.txt')


>
>BUILDID_FILE = open( os.path.join( args['depth'] + "/config/buildid"), 'r')

note my example above, you want a comma there in your call to os.path.join, not a +.

>buildid = BUILDID_FILE.readline()
>BUILDID_FILE.close()

it's more conventional Python to write:
with open(os.path.join(args['depth'], 'config/buildid'), 'r') as f:
  buildid = f.readline()

>
>#3) Read $srcdir/module.ver if it exists for some override values
>if os.path.isfile( os.path.join( args['srcdir'] + '/module.ver')): # Check module.ver file exists...
>    # It exists!!! YAAAAAYYYYYY......
>    # Read the Module.ver file
>    VER_FILE = open( os.path.join( args['srcdir'] + '/module.ver'), 'r')
>    # Complicated reading...Involes RE.... Hence Do it later...
>
>
>else:
>    # It does'nt exist... Take that suckers....
>    # Use default values.
>    # Where am i gonna find these values???
>    # Got It!!!
>    if args['quiet'] is not None:
>        # Shout it out soapbox!!!
>        print "WARNING: No module.ver file included ($module, $binary). Default values used\n"

As funny as your stream-of-consciousness comments are here, make sure you remove them when you submit a patch that's closer to being ready to land. :)

>
>#4) Calculate a few trivial values based on the inputs from the commandline and the files in steps 2 and 3
>day_count = getDaysFromBuild()
>
>if milestone is None:
>    milestone = getOfficialMilestone(MILESTONE_FILE);
>
>
>#5) Write to $objdir/module.rc:
>RC_FILE = open( os.path.join( args['objdir'] + '/module.rc'), 'w')
>
>RC_FILE.write(  """
>// This Source Code Form is subject to the terms of the Mozilla Public
>// License, v. 2.0. If a copy of the MPL was not distributed with this
>// file, You can obtain one at http://mozilla.org/MPL/2.0/.
>
>#include<winver.h>
>
>// Note: if you contain versioning information in an included
>// RC script, it will be discarded
>// Use module.ver to explicitly set these values
>
>// Do not edit this file. Changes won\'t affect the build.
>""")
>versionlevel    = 0
>insideversion   = 0
>
>RC_FILE.write(  """
>
>/////////////////////////////////////////////////////////////////////////////
>//
>// Version
>//
>
>1 VERSIONINFO
> FILEVERSION    $fileversion
> PRODUCTVERSION $productversion
> FILEFLAGSMASK 0x3fL
> FILEFLAGS $fileflags
> FILEOS VOS__WINDOWS32
> FILETYPE VFT_DLL
> FILESUBTYPE 0x0L
>BEGIN
>    BLOCK "StringFileInfo"
>    BEGIN
>        BLOCK "000004b0"
>        BEGIN
>            VALUE "Comments", "$comment"
>            VALUE "LegalCopyright", "$copyright"
>            VALUE "CompanyName", "$company"
>            VALUE "FileDescription", "$description"
>            VALUE "FileVersion", "$mfversion"
>            VALUE "ProductVersion", "$mpversion"
>            VALUE "InternalName", "$module"
>            VALUE "LegalTrademarks", "$trademarks"
>            VALUE "OriginalFilename", "$binary"
>            VALUE "ProductName", "$productname"
>            VALUE "BuildID", "$buildid"
>        END
>    END
>    BLOCK "VarFileInfo"
>    BEGIN
>        VALUE "Translation", 0x0, 1200
>    END
>END
>""")

You're going to want to use Python string interpolation here to get the variables into that string you're writing, so it'd look something like:
RC_FILE.write(  """
...
 FILEVERSION    {fileversion}
...
""".format(fileversion=version))

>
>RC_FILE.close()

You should also use a with statement here like I suggested above.

Overall you're off to a good start here, I'd like to see you make more progress. Thanks for working on this!

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hey, are you planning to finish this bug or should I?

Comment 9

[Timur Timirkhanov]

Since there no one seems to work on this bug for four months, and the author of the original patch haven't responded on commends for two months, Шэв like to work on it.

Comment 10

[Timur Timirkhanov]

Attachment: version_win.py replacement for version_win.pl - reviison 1

I tried to address everything in comment #7

Comment 11

[Gregory Szorc [:gps]]

Attachment: Rewrite version_win in Python

I converted your code to a patch file, which is Mozilla's standard for
reviewing code.

I also made some minor changes to styling and did things like add line
breaks to make things slightly more readable.

The changes were invasive enough that I don't feel comfortable reviewing
my own code. Over to ted for review.

Comment 12

[Timur Timirkhanov]

>I also made some minor changes to styling and did things like add line
>breaks to make things slightly more readable.

I do not undersatnd difference between importing os.path and os.
Second, what's reasoning behind re-ordering imports?

Comment 13

[Gregory Szorc [:gps]]

`import os` will pull in `os.path` automatically, so there is no need to do `import os.path`.

I like ordered imports for readability. Makes it clear where to insert new things.

Comment 14

[Gregory Szorc [:gps]]

ted: review ping?

Comment 15

[Gregory Szorc [:gps]]

ted: review ping?

Comment 16

[Adrian Zatreanu]

Hey, I'd be interested to take this up if noone else is still working on it. Will wait for a response before working :)

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8620440 [details] [diff] [review]
Rewrite version_win in Python

Review of attachment 8620440 [details] [diff] [review]:
-----------------------------------------------------------------

::: config/version_win.py
@@ +12,5 @@
> +from mozbuild import milestone
> +
> +
> +def days_from_build_id(build):
> +    init = datetime.datetime(2000, 1, 1, 0, 0, 0)

You can drop the last three arguments.

Comment 18

[Gregory Szorc [:gps]]

I'm not actively working on this.

Comment 19

[shivambalikondwar]

i am an undergrad in engineering , looking forward to help open-source orgs,i have goals to help mozilla through gsoc 
can i solve it ,i have tried to write some python code (web-scrapers) so i am quite good with regex and file handling and as i opened the file it seems easy  lately
so :)

Comment 20

[Valerian Garleanu]

Hello, I am a student in computer science and I want to take on this bug, I am already halfway through with rewriting the perl script. Could I please get someone to assign me this bug. Thanks a lot!

Comment 21

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Hey Valerian (and others), the policy is that we'll assign the bug to you as soon as you upload the patch. Good luck!

Comment 22

[Valerian Garleanu]

Attachment: version_win.patch

version_win.py revision 1

Comment 23

[Valerian Garleanu]

Could I please get a review of the following first revision patch. It is a work in progress. Thanks a lot!(In reply to Valerian Garleanu from comment #22)
> Created attachment 9011796 [details] [diff] [review]
> version_win.patch
> 
> version_win.py revision 1

Comment 24

[Valerian Garleanu]

Attachment: version_win2.patch

Please take a look at this patch instead as I've just realized that I have generated the patch file the wrong way. Sorry for the inconvenience.

Comment 25

[Gregory Szorc [:gps]]

Comment on attachment 9011802 [details] [diff] [review]
version_win2.patch

Looks like this patch/review request fell through the cracks. I'm really sorry about that.

I've added this to my review queue and will look at it.

I just got back from a few weeks away from work and my review queue is a bit long. It could take me a few days to get to it. But I will get to it.

Again, sorry it took so long for someone to notice the patch.

Comment 26

[haotian xiang]

Attachment: version_win.py

I updated some code based on Ted Mielczarek's comments for talinpaul.

Comment 27

[haotian xiang]

(In reply to undefined from comment #undefined)
> 

I deleted some unnecessary arguments so original file can accept that, updated command line arguments and added file headers.

Comment 28

[cbseadarsh]

Hi, I am new to the community. I am an experienced Python programmer. Can someone please guide me on how to proceed.

Comment 29

[Ji Sook Moon]

Hi! Could I be assigned this issue? :) Thank you!

Comment 30

[987ayush]

Hi. Just wanted to know if this issue is still open? If it is, can I be assigned to it?

Comment 31

[Harnaman Kaur]

Hi. Is this issue still open? If yes, I'd be keen to work on it

Comment 32

[Mike Hommey [:glandium]]

Attachment: Bug 1116553 - Rewrite version_win.pl in Python.

This is not a feature-for-feature rewrite. The python version removes
unused things, and simplifies some others:

• Only two command line arguments are taken in, and all the others are
  dropped and the corresponding values are gotten from the buildconfig
  module instead. The command line arguments are also taken as
  positional arguments rather than going with a full argument parser.

• Variable expansion in module.ver used to be limited to one specific
  variable to expand for a given value, which is now replaced with the
  possibility to expand any of the variables that are allowed in
  module.ver.

• The perl version was adding a RT_MANIFEST entry on its own if a
  manifest file existed in the objdir for the given binary, but if such
  a file existed, the build would fail after linking from the changes in
  bug 1613799.

• The perl version was defaulting the module name to the binary name in
  a branch that was never taken because the module name was assigned to
  an empty string before that.

The output from the new script has been validated to being identical to
the output from the perl script, except for one extra whitespace at the
end of a comment.

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7823460f5239
Rewrite version_win.pl in Python. r=firefox-build-system-reviewers,rstewart

Comment 34

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/7823460f5239