Open Bug 1127801 Opened 5 years ago Updated Last year

milestone.py's functions should be able to default the path to milestone.txt

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

People

(Reporter: ted, Assigned: naru.aditya, Mentored)

References

Details

(Whiteboard: [lang=python])

Attachments

(1 file, 10 obsolete files)

milestone.py is nice, but for importing and using it as a module elsewhere it'd be nicer if we didn't have to pass the path to milestone.txt and it could just grab the current environment and use the right path by default.
Mentor: ted
Whiteboard: [lang=python]
ted,i would really like to work on this bug .since I m new to open source and i think this can help me to further venture in open source world.
Hi vaibhav! The first steps here would be to get the Firefox code and build it:
https://developer.mozilla.org/en-US/docs/Simple_Firefox_build

Let me know when you've got that done and I can help you further. You can also visit irc.mozilla.org #introduction for help.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> Hi vaibhav! The first steps here would be to get the Firefox code and build
> it:
> https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> 
> Let me know when you've got that done and I can help you further. You can
> also visit irc.mozilla.org #introduction for help.

Hello, I'm also interested in working on this. I too am totally new to Mozilla! I've got the Firefox code and built it; if you could help me on how I should proceed next, it would be great!
Thanks a lot!
Great! This is the code in question:
https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/milestone.py

You can invoke this script like this:
$ ./mach python -mmozbuild.milestone --topsrcdir .
38.0a1

This bug is asking you to fix the script so you can just do:
$ ./mach python -mmozbuild.milestone

and get the same answer. You should remove the --topsrcdir argument for the script and change get_official_milestone to do something like:
def get_official_milestone(path=None):
    if path is None:
        # Your new code here!

You should be able to use MozbuildObject.from_environment() like this script does:
https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/build/pgo/profileserver.py#l28

Let me know if you have further questions!
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #4)
> Great! This is the code in question:
> https://dxr.mozilla.org/mozilla-central/source/python/mozbuild/mozbuild/
> milestone.py
> 
> You can invoke this script like this:
> $ ./mach python -mmozbuild.milestone --topsrcdir .
> 38.0a1
> 
> This bug is asking you to fix the script so you can just do:
> $ ./mach python -mmozbuild.milestone
> 
> and get the same answer. You should remove the --topsrcdir argument for the
> script and change get_official_milestone to do something like:
> def get_official_milestone(path=None):
>     if path is None:
>         # Your new code here!
> 
> You should be able to use MozbuildObject.from_environment() like this script
> does:
> https://hg.mozilla.org/mozilla-central/annotate/2cb22c058add/build/pgo/
> profileserver.py#l28
> 
> Let me know if you have further questions!

I've done this edit and $ ./mach python -mmozbuild.milestone returns the same answer. I don't know if that is what was required though. How do I proceed further from here?
That sounds great! The next step is to attach your patch here and ask for review. You can use "hg diff" to produce a diff, or "hg export" if you've used "hg commit" or if you're using mercurial queues and then use the "Add an attachment" link on this bug page to attach it.
Attached patch Posible fix for bug 1127801 (obsolete) — Splinter Review
Comment on attachment 8564763 [details] [diff] [review]
Posible fix for bug 1127801

Thanks! What I'd really like to see is a diff though. Like I said previously you can use `hg diff` to produce one.
 from __future__ import print_function, unicode_literals
+from mozbuild.base import MozbuildObject
 
 import argparse
 import os
@@ -26,7 +27,7 @@
     """
     Returns the contents of the first line in `path` that starts with a digit.
     """
-
+    
     with open(path) as fp:
         for line in fp:
             line = line.strip()
@@ -48,11 +49,10 @@
     parser = argparse.ArgumentParser()
     parser.add_argument('--uaversion', default=False, action='store_true')
     parser.add_argument('--symbolversion', default=False, action='store_true')
-    parser.add_argument('--topsrcdir', metavar='TOPSRCDIR', required=True)
     options = parser.parse_args(args)
-
-    milestone_file = os.path.join(options.topsrcdir, 'config', 'milestone.txt')
-
+    
+    build = MozbuildObject.from_environment()
+    milestone_file = os.path.join(build.topsrcdir, 'config', 'milestone.txt')
     milestone = get_official_milestone(milestone_file)
 
     if options.uaversion:

this is the command line output for "hg diff". I'm sorry but I'm new to this, I know you're being kind and understand and explaining to me the silliest of the things. Thank You!
It's no problem, we all have to start somewhere! What you want to do is run that same command, but redirect it to a file that you can upload. Try this:
hg diff > bug1127891.patch

Then locate the bug1127891.patch in your mozilla source directory and attach it using the "Add an Attachment" link here. You'll want to check the checkbox that says "patch" next to the "Content Type" section, and under "Flags", click the dropdown menu next to "review", set it to "?" and then type :ted.mielczarek in the input box that appears next to it (you might want to copy and paste that, it's hard to type!). Then when you submit your patch will be attached and I will get a review request for it.
Attached patch bug1127801.patch (obsolete) — Splinter Review
Hope I've got it right this time round!
Attachment #8565984 - Flags: review?(ted)
Comment on attachment 8565984 [details] [diff] [review]
bug1127801.patch

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

This is almost perfect! I'd like to see it just *slightly* differently though. Thanks for the patch!

Since your next patch is likely to be ready for check-in you might want to read over the docs on how to generate a patch with all the information necessary for someone else to land it for you:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

::: python/mozbuild/mozbuild/milestone.py
@@ +56,1 @@
>      milestone = get_official_milestone(milestone_file)

This is correct, but I'd like this to be in the get_official_milestone function. You should make the path parameter optional, like:
def get_official_milestone(path=None):

Then if it's missing you can do what you did here:
if path is None:
    ... your new code

And you can just remove the milestone_file parameter to the call right here since it'll use that new code.
Attachment #8565984 - Flags: review?(ted)
Made the requisite changes and created a patch as per the guidelines.
Attachment #8564763 - Attachment is obsolete: true
Attachment #8565984 - Attachment is obsolete: true
Attachment #8567448 - Flags: checkin?(ted)
Attached patch Patch v2 (obsolete) — Splinter Review
Would this patch be better suited?
Attachment #8567449 - Flags: checkin?(ted)
Attachment #8567449 - Flags: checkin?(ted)
Attachment #8567448 - Flags: checkin?(ted)
These patches look the same as the previous patch. Am I missing something or did you attach the wrong patch? I asked for some minor changes in comment 12, if you could do those we can definitely get this landed.
Attached patch bug1127801.patch (obsolete) — Splinter Review
Yup! Wrong patch attached!
Attachment #8567448 - Attachment is obsolete: true
Attachment #8567449 - Attachment is obsolete: true
Attachment #8581615 - Flags: checkin?(ted)
Attached patch bug1127801.patch (obsolete) — Splinter Review
Attachment #8581615 - Attachment is obsolete: true
Attachment #8581615 - Flags: checkin?(ted)
Attachment #8581616 - Flags: checkin?(ted)
Assignee: nobody → naru.aditya
Keywords: checkin-needed
Comment on attachment 8581616 [details] [diff] [review]
bug1127801.patch

For future reference, the checkin-needed bug keyword is the preferred way of requesting checkin. Thanks :)
Attachment #8581616 - Flags: checkin?(ted)
Attached patch bug1127801.patch (obsolete) — Splinter Review
Hope got it right this time round!
Attachment #8581616 - Attachment is obsolete: true
Attachment #8581888 - Flags: checkin?(ryanvm)
Keywords: checkin-needed
Comment on attachment 8581888 [details] [diff] [review]
bug1127801.patch

Just checkin-needed is fine :)
Attachment #8581888 - Flags: checkin?(ryanvm)
patching file python/mozbuild/mozbuild/milestone.py
bad hunk #1 @@ -4,6 +4,8 @@
 (7 6 8 8)
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> patching file python/mozbuild/mozbuild/milestone.py
> bad hunk #1 @@ -4,6 +4,8 @@
>  (7 6 8 8)

What is this?
It means your patch is malformed and can't be imported by hg.
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #24)
> It means your patch is malformed and can't be imported by hg.

Okay, do I recreate the patch or is something else to be done?
Yes, I'm not sure what happened, but the patch needs to be in an applicable format to land.
Attached patch bug1127801.patch (obsolete) — Splinter Review
New. Clean build + new changes from which patch was created. Do let me know if this is flawed!
Attachment #8581888 - Attachment is obsolete: true
Keywords: checkin-needed
I don't actually believe this is the cause of that bustage, but I just noticed one other thing that needs to be fixed before we can re-land. This patch *would* cause bustage on clobber builds, because we didn't fix the callers to stop passing --topsrcdir even though we removed it as an argument! It only gets called in a handful of places:
https://dxr.mozilla.org/mozilla-central/source/configure.in#1923
https://dxr.mozilla.org/mozilla-central/source/js/src/configure.in#699
https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/Makefile.in#16

But you'll need to remove the --topsrcdir bit from each of those invocations.
Flags: needinfo?(ted)
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #30)
> I don't actually believe this is the cause of that bustage, but I just
> noticed one other thing that needs to be fixed before we can re-land. This
> patch *would* cause bustage on clobber builds, because we didn't fix the
> callers to stop passing --topsrcdir even though we removed it as an
> argument! It only gets called in a handful of places:
> https://dxr.mozilla.org/mozilla-central/source/configure.in#1923
> https://dxr.mozilla.org/mozilla-central/source/js/src/configure.in#699
> https://dxr.mozilla.org/mozilla-central/source/toolkit/xre/Makefile.in#16
> 
> But you'll need to remove the --topsrcdir bit from each of those invocations.

I was thinking of keeping the argument but making it not required. What do you think?
Flags: needinfo?(naru.aditya)
Attached patch final.patch (obsolete) — Splinter Review
Made the argument --topsrcdir optional. This ought to pass all the tests.
Attachment #8582491 - Attachment is obsolete: true
Keywords: checkin-needed
Comment on attachment 8587299 [details] [diff] [review]
final.patch

That's fine, although it'd be nice to remove it since it's not needed. We can deal with that cleanup in a follow-up though.
Attachment #8587299 - Flags: review+
I'd feel much better about re-landed this with a Try push first.
Attached patch bug1127801_final.patch (obsolete) — Splinter Review
Removed --topsrcdir arguments. Don't know if this is correct!
Attachment #8587367 - Flags: review?(ted)
Comment on attachment 8587367 [details] [diff] [review]
bug1127801_final.patch

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

Almost! Fix up the one issue here and I'll r+ and we can push this to try for a sanity check.

::: configure.in
@@ +1911,5 @@
>  
>  dnl ==============================================================
>  dnl Get mozilla version from central milestone file
>  dnl ==============================================================
> +MOZILLA_VERSION=`$PYTHON $srcdir/python/mozbuild/mozbuild/milestone.py $srcdir`

You can drop the $srcdir bit entirely on these lines.

::: js/src/configure.in
@@ +235,5 @@
>  
>  dnl ==============================================================
>  dnl Get mozilla version from central milestone file
>  dnl ==============================================================
> +MOZILLA_VERSION=`$PYTHON $srcdir/python/mozbuild/mozbuild/milestone.py $srcdir`

Same here.
Attachment #8587367 - Flags: review?(ted) → review-
The requested changes have been implemented.
Attachment #8587299 - Attachment is obsolete: true
Attachment #8587367 - Attachment is obsolete: true
Attachment #8588822 - Flags: review?(ted)
Comment on attachment 8588822 [details] [diff] [review]
bug1127801v3.patch

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

Looks good! I'll run this by try.
Attachment #8588822 - Flags: review?(ted) → review+
Try builds look green, this should be good to go.
Keywords: checkin-needed
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #40)
> Try builds look green, this should be good to go.

Wonderful!
Backed out for mass test bustage again. Ted - when I asked for a Try push, I assumed you were going to run tests as well since that's what broke last time, not builds.
https://hg.mozilla.org/integration/mozilla-inbound/rev/85dce713d067
This may actually be an infra issue. Re-landed to see what happens.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6bbe2e6958a9
Still hitting the Mulet bustage after re-landing. Backed out again.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6288bbcd0ab2
lightsofapollo managed to spot this in the build logs of the failed runs:
"1428427562977	addons.xpi	WARN	Ignoring invalid targetApplication entry in install manifest"
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #47)
> lightsofapollo managed to spot this in the build logs of the failed runs:
> "1428427562977	addons.xpi	WARN	Ignoring invalid targetApplication entry in
> install manifest"

It's just a warning and not an error right? and what does this indicate?
The warning above indicates something but the real error here is:

>JavaScript error: http://mochi.test:8888/tests/SimpleTest/setup.js, line 110: ReferenceError: >SpecialPowers is not defined ?

I am not sure of the cause but mochitests will not run without special powers.
Product: Core → Firefox Build System
(In reply to naru.aditya from comment #3)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #2)
> > Hi vaibhav! The first steps here would be to get the Firefox code and build
> > it:
> > https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
> > 
> > Let me know when you've got that done and I can help you further. You can
> > also visit irc.mozilla.org #introduction for help.
> 
> Hello, I'm also interested in working on this. I too am totally new to
> Mozilla! I've got the Firefox code and built it; if you could help me on how
> I should proceed next, it would be great!
> Thanks a lot!
You need to log in before you can comment on or make changes to this bug.