Make more paths in moz.build follow the relative/absolute rules

RESOLVED FIXED in Firefox 41

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

unspecified
mozilla41
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(10 attachments, 1 obsolete attachment)

39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
39 bytes, text/x-review-board-request
gps
: review+
Details
Not sure the summary is clear, but here it is in a more verbose form:

We have a few variables in moz.builds (like LOCAL_INCLUDES) that take paths in relative or absolute forms, where the absolute form resolves to a path relative to the top source directory (or top object directory for some other variables).

This pattern should be generalized to all variables that contains paths.

While at it, we may want to change the rule to be:
- Starting with '/' is a filesystem absolute path
- Starting with '//' is a topsrcdir/topobjdir relative path
- Otherwise it is a srcdir/curdir relative path.

(as mentioned in another bug)

I won't work immediately on this, but if someone wants to jump in the build system code to implement this before I do, this certainly can be a nice first bug.
Whiteboard: [good first bug] → [good first bug][mentor=glandium][lang=python]
I am want to start with firefox dev tools. Can someone please assigned this bug to me, if its not yet fixed. I want to solve bugs related to UI and also I want to learn about and work with systems, can someone help me.
(In reply to amanjain110893 from comment #1)
> I am want to start with firefox dev tools. Can someone please assigned this
> bug to me, if its not yet fixed. I want to solve bugs related to UI and also
> I want to learn about and work with systems, can someone help me.

This bug is about the scripts that help building firefox, not the firefox UI or the devtools. Are you still interested? If you are, do you know python?
I'm interested in solving this bug. Please assinged it to me.
(In reply to Thomas from comment #3)
> I'm interested in solving this bug. Please assinged it to me.

We don't assign bugs to newcomers in bugzilla, but please feel free to work on it and attach a patch.
Mentor: mh+mozilla
Whiteboard: [good first bug][mentor=glandium][lang=python] → [good first bug][lang=python]
Can someone please paste the source code repository link?
How to get the repository link in general for any bug in bugzilla? Is it provided in the bug description?
Also, if there is any product to repository mapping somewhere, please let me know.

Thanks!
I want to work on this bug. I will submit a patch to it ASAP.
Depends on: 1062221
Attachment 8484062 [details] [diff] from bug 1062221 paves the way to implement this. The SourcePath class added in that patch is a type that can be used for many of the sandbox variables that are always source paths. We'd need a class for paths always in the objdir, and probably another deriving from SourcePath that also takes objdir paths when it starts with another special character (I think we were saying '~' in some other bug).

I wouldn't advise starting on this bug until attachment 8484062 [details] [diff] [review] is reviewed, though.

Note I'm not entirely sure about the '/' vs. '//' thing. On one hand the proposed scheme in comment 0 would make us use the same semantics as gyp, on the other hand, the opposite ('//' for file-system absolute paths) would minimize the required changes. Greg, what do you think about this?
Flags: needinfo?(gps)
Bug 976733 also has a patch to establish some path normalization foo. Both that and bug 1062221 are in my review queue. I guess I need to sort out this mess.

Let's put this bug on hold until things are resolved.
Depends on: 976733
Flags: needinfo?(gps)
blocking-b2g: 2.2? → ---
Hi. I'd like to work on this bug. Can you please guide me as to how I go about it?
Greg, what's the status here now?
Flags: needinfo?(gps)
I think comment #7 is a good summary of where we are.

It still looks like there may be some bikeshedding over the format. If so, this really shouldn't be a mentored bug since we don't know what we're doing yet :/ Maybe glandium had something else in mind? He is the mentor here...
Flags: needinfo?(gps) → needinfo?(mh+mozilla)
hi, I would like to work on this bug.I have set up my firefox build environment and am also good with python. Can you please guide me to go about it ?
The bug is resolved right now, as a good first bug can people here start working on it?
Turns out this is not exactly trivial. There are many intricacies. I should have done a better job at answering this bug, too. Anyways, as this will help simplify some other things I have cooking, I'm going to pick this.
Assignee: nobody → mh+mozilla
Mentor: mh+mozilla
Flags: needinfo?(mh+mozilla)
Whiteboard: [good first bug][lang=python]
/r/9083 - Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend
/r/9085 - Bug 991983 - Emit absolute paths for UnifiedSources
/r/9087 - Bug 991983 - Emit absolute paths for other sources
/r/9089 - Bug 991983 - Add a ContextDerivedTypedListWithItems type
/r/9091 - Bug 991983 - Don't pass template paths to contexts
/r/9093 - Bug 991983 - Remove commented code in gyp_reader.py
/r/9095 - Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests
/r/9097 - Bug 991983 - Define SOURCES as SourcePath
/r/9099 - Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES
/r/9101 - Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Pull down these commits:

hg pull -r 68361d977bd93a53ce40bf32ac9e2058118fd82f https://reviewboard-hg.mozilla.org/gecko/
Attachment #8607913 - Flags: review?(gps)
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

/r/9083 - Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend
/r/9085 - Bug 991983 - Emit absolute paths for UnifiedSources
/r/9087 - Bug 991983 - Emit absolute paths for other sources
/r/9089 - Bug 991983 - Add a ContextDerivedTypedListWithItems type
/r/9091 - Bug 991983 - Don't pass template paths to contexts
/r/9093 - Bug 991983 - Remove commented code in gyp_reader.py
/r/9095 - Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests
/r/9097 - Bug 991983 - Define SOURCES as SourcePath
/r/9099 - Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES
/r/9101 - Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Pull down these commits:

hg pull -r 68361d977bd93a53ce40bf32ac9e2058118fd82f https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

/r/9083 - Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend
/r/9085 - Bug 991983 - Emit absolute paths for UnifiedSources
/r/9087 - Bug 991983 - Emit absolute paths for other sources
/r/9089 - Bug 991983 - Add a ContextDerivedTypedListWithItems type
/r/9091 - Bug 991983 - Don't pass template paths to contexts
/r/9093 - Bug 991983 - Remove commented code in gyp_reader.py
/r/9095 - Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests
/r/9097 - Bug 991983 - Define SOURCES as SourcePath
/r/9099 - Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES
/r/9101 - Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Pull down these commits:

hg pull -r 85ff9c1b82e0943ef55bf98e3453bc77e20d4d79 https://reviewboard-hg.mozilla.org/gecko/
(In reply to Mike Hommey [:glandium] from comment #18)
> hg pull -r 85ff9c1b82e0943ef55bf98e3453bc77e20d4d79
> https://reviewboard-hg.mozilla.org/gecko/

Review board appears to have an inconsistent state here... that changeset is not what it gives a diff for.
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

/r/9083 - Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend
/r/9085 - Bug 991983 - Emit absolute paths for UnifiedSources
/r/9087 - Bug 991983 - Emit absolute paths for other sources
/r/9089 - Bug 991983 - Add a ContextDerivedTypedListWithItems type
/r/9091 - Bug 991983 - Don't pass template paths to contexts
/r/9093 - Bug 991983 - Remove commented code in gyp_reader.py
/r/9095 - Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests
/r/9097 - Bug 991983 - Define SOURCES as SourcePath
/r/9099 - Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES
/r/9101 - Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Pull down these commits:

hg pull -r ab58f8726c1da5726fac6f140e066a574c4b7204 https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/9087/#review7879

I almost wonder if we should be doing path normalization inside contexts. Although, that may increase run-time overhead. Perhaps we could teach all context variables holding path info to have methods that expose absolute paths? It seems easier to add an attribute to variable types than to type mozpath.join in dozens of places. Just a thought.
https://reviewboard.mozilla.org/r/9087/#review7881

Perhaps I should read the rest of your commits first :)
https://reviewboard.mozilla.org/r/9091/#review7885

::: python/mozbuild/mozbuild/frontend/sandbox.py:197
(Diff revision 1)
> +            if not becomes_current_path:

I was initially confused by this. Please add a comment saying we need to add the current file since it wasn't added to the stack earlier.
https://reviewboard.mozilla.org/r/9095/#review7937

This looks mostly good. But I'd like to take another look with comments addressed before I grant review.

::: python/mozbuild/mozbuild/frontend/context.py:391
(Diff revision 1)
> +    def __eq__(self, other):
> +        return self.__cmp__(other) == 0
> +
> +    def __ne__(self, other):
> +        return self.__cmp__(other) != 0
> +
> +    def __lt__(self, other):
> +        return self.__cmp__(other) < 0
> +
> +    def __gt__(self, other):
> +        return self.__cmp__(other) > 0
> +
> +    def __le__(self, other):
> +        return self.__cmp__(other) <= 0
> +
> +    def __ge__(self, other):
> +        return self.__cmp__(other) >= 0

Are these needed? I thought the defaults were implemented in terms of `__cmp__` already.

::: python/mozbuild/mozbuild/frontend/context.py:473
(Diff revision 1)
> +            class _TypeMeta(metaclass):
> +                def __call__(cls, value):
> +                    return super(_TypeMeta, cls).__call__(context, value)
> +            class _Type(klass):
> +                __metaclass__ = _TypeMeta

My head just exploded.

This is complicated enough to warrant some inline comments.

::: python/mozbuild/mozbuild/test/frontend/test_context.py:278
(Diff revision 1)
> +        config.topsrcdir = mozpath.abspath(os.curdir)

`os.curdir` is '.'. I /thought/ we normalized topsrcdir to absolute by the time it hit the config object. You may be testing conditions that don't happen in the wild.

::: python/mozbuild/mozbuild/test/frontend/test_context.py:366
(Diff revision 1)
> +                         mozpath.join(config.topobjdir, 'qux', 'qux'))
> +
> +        path2 = Path(path2)

It doesn't hurt to throw a `self.assertEqual(path1, path2)` in here.

::: python/mozbuild/mozbuild/test/frontend/test_context.py:454
(Diff revision 1)
> +    @unittest.skip('')
> +    def test_rebase_path(self):

It's not obvious to me right now why you are skipping this test. I'm assuming you will get back to this in a subsequent commit. But please leave a comment for things like this in the future.

Opening an issue in case the skip is accidental.
https://reviewboard.mozilla.org/r/9097/#review7943

::: toolkit/profile/moz.build:17
(Diff revision 1)
> -UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/nsProfileLock.cpp' ]
> +UNIFIED_SOURCES += [ '/profile/dirserviceprovider/nsProfileLock.cpp' ]
>  
>  if CONFIG['OS_ARCH'] == 'WINNT':
> -    UNIFIED_SOURCES += [ TOPSRCDIR + '/profile/dirserviceprovider/ProfileUnlockerWin.cpp' ]
> +    UNIFIED_SOURCES += [ '/profile/dirserviceprovider/ProfileUnlockerWin.cpp' ]

While you're here, can you please cuddle the braces?
https://reviewboard.mozilla.org/r/9099/#review7945

One less variable. Excellent!

::: dom/plugins/ipc/moz.build:72
(Diff revision 2)
> +        '!moc_NestedLoopTimer.cpp',

Due to sorting requirements, I guess this means all objdir entries will appear first now. I don't think that's a problem. But I just wanted to make that observation in case others have any thoughts on this.

We certainly could ignore leading ! from sort comparison. But I'm not convinced the complexity is warranted.

::: media/libopus/moz.build:92
(Diff revision 2)
> -    GENERATED_SOURCES += [ '%s.%s' % (f, CONFIG['ASM_SUFFIX']) for f in [
> +    SOURCES += [ '!celt_pitch_xcorr_arm-gnu.%s' % CONFIG['ASM_SUFFIX'] ]

Please cuddle braces while you're here.

::: python/mozbuild/mozbuild/frontend/emitter.py:720
(Diff revision 2)
> -                       'GENERATED_SOURCES'):
> -            srcs = all_sources[symbol] = []
> +            srcs = sources[symbol] = []
> +            gen_srcs = gen_sources[symbol] = []

make `sources` and `gen_sources` a defaultdict?

::: python/mozbuild/mozbuild/frontend/emitter.py:727
(Diff revision 2)
> +                else:
> +                    gen_srcs.append(full_path)

I'd feel better if there were an assert on ObjdirPath in here.
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

Excellent series. Mosts commits have r+. Clearing review flag until v2.
Attachment #8607913 - Flags: review?(gps)
(In reply to Gregory Szorc [:gps] from comment #28)
> https://reviewboard.mozilla.org/r/9095/#review7937
> 
> This looks mostly good. But I'd like to take another look with comments
> addressed before I grant review.
> 
> ::: python/mozbuild/mozbuild/frontend/context.py:391
> (Diff revision 1)
> > +    def __eq__(self, other):
> > +        return self.__cmp__(other) == 0
> > +
> > +    def __ne__(self, other):
> > +        return self.__cmp__(other) != 0
> > +
> > +    def __lt__(self, other):
> > +        return self.__cmp__(other) < 0
> > +
> > +    def __gt__(self, other):
> > +        return self.__cmp__(other) > 0
> > +
> > +    def __le__(self, other):
> > +        return self.__cmp__(other) <= 0
> > +
> > +    def __ge__(self, other):
> > +        return self.__cmp__(other) >= 0
> 
> Are these needed? I thought the defaults were implemented in terms of
> `__cmp__` already.

unicode comes with those methods attached, and when a class has those methods, __cmp__ is not used (except for cmp())

> ::: python/mozbuild/mozbuild/test/frontend/test_context.py:278
> (Diff revision 1)
> > +        config.topsrcdir = mozpath.abspath(os.curdir)
> 
> `os.curdir` is '.'. I /thought/ we normalized topsrcdir to absolute by the
> time it hit the config object. You may be testing conditions that don't
> happen in the wild.

That's a plain copy of the other tests in the same file. But iirc, our normalization happens in the actual ConfigEnvironment objects, and this is using a mock object. Anyways, if cleanup there is to be done here, can this be a followup?

> ::: python/mozbuild/mozbuild/test/frontend/test_context.py:454
> (Diff revision 1)
> > +    @unittest.skip('')
> > +    def test_rebase_path(self):
> 
> It's not obvious to me right now why you are skipping this test. I'm
> assuming you will get back to this in a subsequent commit. But please leave
> a comment for things like this in the future.
> 
> Opening an issue in case the skip is accidental.

Err, it's partly accidental. It comes from the test having been written for another patch queue that had problems, but in some of the things it did, Path(context, path2) would rebase the value of the string itself from the context in path2 to the context given. I didn't reimplement this, so I initially just skipped the test waiting to see whether I'd implement it or not. I didn't, but didn't remove the test. Now that I think about it, the lack of rebasing leads to an inconsistency.

That is, Path(<Context /topsrcdir/bar>, <Path (/topsrcdir/foo)foo>) returns a <Path (/topsrcdir/bar)foo>. I think it should return either <Path (/topsrcdir/foo)foo> (unmodified path) or <Path (/topsrcdir/bar)../foo/foo> (rebased path). Thoughts?
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

/r/9083 - Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend
/r/9085 - Bug 991983 - Emit absolute paths for UnifiedSources
/r/9087 - Bug 991983 - Emit absolute paths for other sources
/r/9089 - Bug 991983 - Add a ContextDerivedTypedListWithItems type
/r/9091 - Bug 991983 - Don't pass template paths to contexts
/r/9093 - Bug 991983 - Remove commented code in gyp_reader.py
/r/9095 - Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests
/r/9097 - Bug 991983 - Define SOURCES as SourcePath
/r/9099 - Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES
/r/9101 - Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Pull down these commits:

hg pull -r 3eb94ac3ce0498129f4a2db379de6212da1879fc https://reviewboard-hg.mozilla.org/gecko/
Attachment #8607913 - Flags: review?(gps)
https://reviewboard.mozilla.org/r/9095/#review8233

::: python/mozbuild/mozbuild/test/frontend/test_context.py:463
(Diff revision 2)
> +        path1 = Path(ctxt1, 'qux')
> +        path2 = Path(ctxt2, path1)
> +        self.assertEqual(path2, path1)
> +        self.assertEqual(path2, 'qux')
> +        self.assertEqual(path2.full_path,
> +                         mozpath.join(config.topsrcdir, 'foo', 'qux'))

As mentioned on IRC, we should also assert the contexts are equal.
Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

https://reviewboard.mozilla.org/r/9081/#review8241

Interdiff looks sane. Ship all of the things!
Attachment #8607913 - Flags: review?(gps) → review+
Blocks: 1170431
Attachment #8607913 - Attachment is obsolete: true
Attachment #8618105 - Flags: review+
Attachment #8618106 - Flags: review+
Attachment #8618107 - Flags: review+
Attachment #8618108 - Flags: review+
Attachment #8618109 - Flags: review+
Attachment #8618110 - Flags: review+
Attachment #8618111 - Flags: review+
Attachment #8618112 - Flags: review+
Attachment #8618113 - Flags: review+
Attachment #8618114 - Flags: review+
Duplicate of this bug: 964302
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.