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

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

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.

Comment 1

[amanjain110893]

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.

Comment 2

[Mike Hommey [:glandium]]

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

Comment 3

[Thomas]

I'm interested in solving this bug. Please assinged it to me.

Comment 4

[Mike Hommey [:glandium]]

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

Comment 5

[Harsh]

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!

Comment 6

[Amit Tripathi[:amit001]]

I want to work on this bug. I will submit a patch to it ASAP.

Comment 7

[Mike Hommey [:glandium]]

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?

Comment 8

[Gregory Szorc [: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.

Comment 10

[Abhilash Panigrahi]

Hi. I'd like to work on this bug. Can you please guide me as to how I go about it?

Comment 11

[Josh Matthews [:jdm]]

Greg, what's the status here now?

Comment 12

[Gregory Szorc [: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...

Comment 13

[Yash Mehrotra]

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 ?

Comment 14

[naru.aditya]

The bug is resolved right now, as a good first bug can people here start working on it?

Comment 15

[Mike Hommey [:glandium]]

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.

Comment 16

[Mike Hommey [:glandium]]

Attachment: 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 17

[Mike Hommey [:glandium]]

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 18

[Mike Hommey [:glandium]]

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/

Comment 19

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

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/

Comment 21

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9083/#review7875

Ship It!

Comment 22

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9085/#review7877

Ship It!

Comment 23

[Gregory Szorc [:gps]]

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.

Comment 24

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9087/#review7881

Perhaps I should read the rest of your commits first :)

Comment 25

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9089/#review7883

Ship It!

Comment 26

[Gregory Szorc [:gps]]

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.

Comment 27

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9093/#review7887

Ship It!

Comment 28

[Gregory Szorc [:gps]]

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.

Comment 29

[Gregory Szorc [:gps]]

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?

Comment 30

[Gregory Szorc [:gps]]

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 31

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9101/#review7949

Comment 32

[Gregory Szorc [:gps]]

Comment on attachment 8607913 [details]
MozReview Request: bz://991983/glandium

Excellent series. Mosts commits have r+. Clearing review flag until v2.

Comment 33

[Mike Hommey [:glandium]]

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

[Mike Hommey [:glandium]]

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/

Comment 35

[Gregory Szorc [: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 36

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/9095/#review8235

Ship It!

Comment 37

[Gregory Szorc [:gps]]

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!

Comment 38

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed70a37d8a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5766bea4a8b8
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f6e2d95039b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c0a14fba2e7
https://hg.mozilla.org/integration/mozilla-inbound/rev/639e3f93e675
https://hg.mozilla.org/integration/mozilla-inbound/rev/193f81e744c2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa6d843f9086
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d46c1cdb8e
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8f1e56cdaf
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6c2ad781550

Comment 39

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f18f26ec14b

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ed70a37d8a4
https://hg.mozilla.org/mozilla-central/rev/5766bea4a8b8
https://hg.mozilla.org/mozilla-central/rev/8f6e2d95039b
https://hg.mozilla.org/mozilla-central/rev/8c0a14fba2e7
https://hg.mozilla.org/mozilla-central/rev/639e3f93e675
https://hg.mozilla.org/mozilla-central/rev/193f81e744c2
https://hg.mozilla.org/mozilla-central/rev/fa6d843f9086
https://hg.mozilla.org/mozilla-central/rev/a2d46c1cdb8e
https://hg.mozilla.org/mozilla-central/rev/bd8f1e56cdaf
https://hg.mozilla.org/mozilla-central/rev/a6c2ad781550
https://hg.mozilla.org/mozilla-central/rev/9f18f26ec14b

Comment 42

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Add a ContextDerivedTypedListWithItems type

Comment 43

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Don't pass template paths to contexts

Comment 44

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Remove commented code in gyp_reader.py

Comment 45

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Refactor SourcePath handling for moz.build, add new classes and extensive tests

Comment 46

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Define SOURCES as SourcePath

Comment 47

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Use objdir-relative SOURCES instead of GENERATED_SOURCES

Comment 48

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Make TEST_HARNESS_FILES use the *Path classes instead of a separate set of methods to resolve paths

Comment 49

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Set GARBAGE for GeneratedSources in the recursivemake backend

Comment 50

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Emit absolute paths for UnifiedSources

Comment 51

[Mike Hommey [:glandium]]

Attachment: MozReview Request: Bug 991983 - Emit absolute paths for other sources