Closed Bug 1116553 Opened 6 years ago Closed 4 months ago

Rewrite version_win.pl in Python

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox81 fixed)

RESOLVED FIXED
81 Branch
Tracking Status
firefox81 --- fixed

People

(Reporter: ted, Assigned: glandium, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=python])

Attachments

(4 files, 3 obsolete files)

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
So i am a beginer as you might have guessed... I've started working on a peice of Python code...
You may wish to build on to of the patch in bug 739601; it already provides some of the code you'll need.
Here is what i've worked on so far... Criticisms are welcome (Dont be too harsh on me...)
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!
Attachment #8543759 - Flags: feedback?(ted)
I'm going to assign this bug to you, which will let other people know you're actively working on it.
Assignee: nobody → 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 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!
Attachment #8543759 - Flags: feedback?(ted) → feedback+
Hey, are you planning to finish this bug or should I?
Flags: needinfo?(talinpaul)
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.
Flags: needinfo?(talinpaul)
I tried to address everything in comment #7
Assignee: talinpaul → tim_tim2000
Attachment #8543759 - Attachment is obsolete: true
Attachment #8608078 - Flags: review?(ted)
Attachment #8608078 - Flags: feedback?(Ms2ger)
Attachment #8608078 - Flags: review?(ted) → review?(gps)
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.
Attachment #8620440 - Flags: review?(ted)
Assignee: tim_tim2000 → gps
Status: NEW → ASSIGNED
Attachment #8608078 - Attachment is obsolete: true
Attachment #8608078 - Flags: review?(gps)
Attachment #8608078 - Flags: feedback?(Ms2ger)
>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?
Flags: needinfo?(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.
Flags: needinfo?(gps)
ted: review ping?
ted: review ping?
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 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.
Attachment #8620440 - Flags: review?(ted) → review+
I'm not actively working on this.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Product: Core → Firefox Build System
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 :)
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!
Hey Valerian (and others), the policy is that we'll assign the bug to you as soon as you upload the patch. Good luck!
Attached patch version_win.patch (obsolete) — Splinter Review
version_win.py revision 1
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
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.
Attachment #9011796 - Attachment is obsolete: true
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.
Attachment #9011802 - Flags: review?(gps)
Attached patch version_win.pySplinter Review
I updated some code based on Ted Mielczarek's comments for talinpaul.
(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.

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

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

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

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

Blocks: 1656422
Assignee: nobody → mh+mozilla

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.

Blocks: 1656141
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/7823460f5239
Rewrite version_win.pl in Python. r=firefox-build-system-reviewers,rstewart
Status: NEW → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 81 Branch
Regressions: 1657328
No longer regressions: 1657328
Regressions: 1657341
Regressions: 1657420
Regressions: 1657333
You need to log in before you can comment on or make changes to this bug.