Detect filesystem and link support in configure

ASSIGNED
Assigned to

Status

()

Core
Build Config
ASSIGNED
7 months ago
5 months ago

People

(Reporter: gps, Assigned: gps)

Tracking

(Blocks: 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 wontfix)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(19 attachments, 1 obsolete attachment)

59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review+
Details | Review
59 bytes, text/x-review-board-request
glandium
: review-
Details | Review
59 bytes, text/x-review-board-request
Details | Review
(Assignee)

Description

7 months ago
Bug 1380416 is looking to explore the use of hard links instead of symlinks for file installation in the build system.

We've historically flirted with the idea. We've also flirted with using links on Windows instead of file copies. We've also had people request we not symlink from objdir to srcdir because it makes copying the objdir to a different filesystem or machine difficult. (It is common for people to want to build on a fast machine and then run things on another.)

Furthermore, we can't easily switch from symlinks to hard links because a) they don't work across filesystems b) some filesystems have buggy hard links behavior (including some filesystems used by Docker IIRC).

The first step towards changing our file installation behavior is detecting what's possible and making it configurable. Let's add that detection to moz.configure.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 29

7 months ago
mozreview-review
Comment on attachment 8888571 [details]
Bug 1382697 - Remove native nsinstall;

https://reviewboard.mozilla.org/r/159552/#review164954

::: config/config.mk:434
(Diff revision 3)
>  ifeq ($(OS_ARCH),Darwin)
>  ifndef NSDISTMODE
>  NSDISTMODE=absolute_symlink
>  endif
>  PWD := $(CURDIR)
>  endif

This series still needs to deal with NSDISTMODE.

As it stands, I'm pretty sure I regressed behavior of NSDISTMODE. If this variable is set to "copy" and the file install mode is symlink, NSDISTMODE is ignored. We'll need to add an override somewhere. I don't think it will be too much work.

Comment 30

7 months ago
mozreview-review
Comment on attachment 8888570 [details]
Bug 1382697 - Teach nsinstall.py to handle symlinks;

https://reviewboard.mozilla.org/r/159550/#review165174

::: config/nsinstall.py:132
(Diff revision 3)
> +                    with open(srcpath, 'rb') as ifh:
> +                        with open(targetpath, 'wb') as ofh:
> +                            ofh.write(ifh.read())

Is there a reason not to use `shutil.copy2` here? It should handle a) doing the copy without reading the entire file into memory, b) copying the stat metadata.
(Assignee)

Comment 31

7 months ago
mozreview-review-reply
Comment on attachment 8888570 [details]
Bug 1382697 - Teach nsinstall.py to handle symlinks;

https://reviewboard.mozilla.org/r/159550/#review165174

> Is there a reason not to use `shutil.copy2` here? It should handle a) doing the copy without reading the entire file into memory, b) copying the stat metadata.

For the code in its current form, shutil's helper functions can be used. I was lazy when I coded this.

However, shutil isn't the most effective way to copy a file! Notably, it doesn't use CopyFile() on Windows. The mozpack code does. I'd like to convert all of this "install a file" code to use mozpack so we can standardize on "install file" implementation. I'll try to hack on that today.

Comment 32

7 months ago
mozreview-review
Comment on attachment 8888428 [details]
Bug 1382697 - Use non-mutable default argument;

https://reviewboard.mozilla.org/r/159368/#review165386

::: commit-message-68046:3
(Diff revision 2)
> +Bug 1382697 - Use non-mutable default argument; r?glandium
> +
> +Noticed this bug while I was looking at the code.

Technically, there is no bug as the argument is only ever read, and there's no reason for it to be written to.
Attachment #8888428 - Flags: review?(mh+mozilla) → review+

Comment 33

7 months ago
mozreview-review
Comment on attachment 8888446 [details]
Bug 1382697 - Handle pattern symlinks in InstallManifestNoSymlinks;

https://reviewboard.mozilla.org/r/159396/#review165388
Attachment #8888446 - Flags: review?(mh+mozilla) → review+

Comment 34

7 months ago
mozreview-review
Comment on attachment 8888429 [details]
Bug 1382697 - Detect symlink and hard link support in configure;

https://reviewboard.mozilla.org/r/159370/#review165392

::: build/moz.configure/init.configure:751
(Diff revision 3)
> +    except OSError:
> +        return None
> +
> +    os.remove(dest)
> +
> +    return 'posix'

until such time we actually something fine grained, just return True.

::: build/moz.configure/init.configure:775
(Diff revision 3)
> +    except OSError:
> +        return None
> +
> +    os.remove(dest)
> +
> +    return 'posix'

likewise

::: build/moz.configure/init.configure:780
(Diff revision 3)
> +def topobjdir_hardlinks(build_env):
> +    source = os.path.join(build_env.topsrcdir, 'mach')
> +    dest = os.path.join(build_env.topobjdir, 'hardlink.test')
> +
> +    return check_hardlink(source, dest)

why isn't this inline like for symlinks? In fact both functions are largely identical, you could write a template instead:

@template
def check_sym_hard_links(sym_or_hard):
    funcname = {
        'symbolic': 'symlink',
        'hard': 'link',
    }[sym_or_hard]
    @depends(check_build_environment)
    @checking('for %s link support from topsrcdir to topobjdir' % sym_or_hard)
    @imports(...)
    def check(build_env):
        func = getattr(os, funcname, None)
        if not func:
            return None

        source = ...
        dest = ... '%slink.test' % sym_or_hard)

        try:
            os.remove(dest)
        ...

        try:
            func(source, dest)
        except OSError:
            return None
        finally:
            os.remove(dest)

        return True

set_config('TOPOBJDIR_TOPSRCDIR_SYMLINKS', check_sym_hard_links('symbolic'))
set_config('TOPOBJDIR_TOPSRCDIR_HARDLINKS', check_sym_hard_links('hard'))

::: build/moz.configure/init.configure:800
(Diff revision 3)
> +
> +    with open(source, 'ab'):
> +        pass
> +
> +    try:
> +        return check_hardlink(source, dest)

Oh, I see why now. There's still opportunity for more code deduplication, though.

::: build/moz.configure/init.configure:804
(Diff revision 3)
> +    try:
> +        return check_hardlink(source, dest)
> +    finally:
> +        os.remove(source)
> +
> +set_config('TOPOBJDIR_INTRA_HARDLINKS', topobjdir_intra_hardlinks)

I don't have better suggestions, but the variable names feel overboard.
Attachment #8888429 - Flags: review?(mh+mozilla)

Comment 35

7 months ago
mozreview-review
Comment on attachment 8888430 [details]
Bug 1382697 - Configure flag to control file installation mode;

https://reviewboard.mozilla.org/r/159372/#review165396

::: build/moz.configure/init.configure:806
(Diff revision 3)
>      finally:
>          os.remove(source)
>  
>  set_config('TOPOBJDIR_INTRA_HARDLINKS', topobjdir_intra_hardlinks)
>  
> +option('--with-file-install-mode',

While "install" in the build system has its terminology, install, in configure/make land usually is loaded and relates to `make install`, and this option name is confusing in that sense.

I'm also not entirely convinced we need to make this overridable.
Attachment #8888430 - Flags: review?(mh+mozilla)

Comment 36

7 months ago
mozreview-review
Comment on attachment 8888431 [details]
Bug 1382697 - Remove --no-symlinks argument from process_install_manifest;

https://reviewboard.mozilla.org/r/159374/#review165398
Attachment #8888431 - Flags: review?(mh+mozilla) → review+

Comment 37

7 months ago
mozreview-review
Comment on attachment 8888432 [details]
Bug 1382697 - Honor file install mode when processing install manifests;

https://reviewboard.mozilla.org/r/159376/#review165400
Attachment #8888432 - Flags: review?(mh+mozilla) → review+

Comment 38

7 months ago
mozreview-review
Comment on attachment 8888447 [details]
Bug 1382697 - Honor file install mode when processing jars;

https://reviewboard.mozilla.org/r/159398/#review165402

::: toolkit/moz.configure:466
(Diff revision 2)
>          return 'flat'
> +
> +    # Only use symlinks if file install mode allows it.
> +    if install_mode == 'symlink':
> -    return 'symlink'
> +        return 'symlink'
> +    else:

no need for an else.
Attachment #8888447 - Flags: review?(mh+mozilla) → review+

Comment 39

7 months ago
mozreview-review
Comment on attachment 8888567 [details]
Bug 1382697 - Reformat nsinstall.py;

https://reviewboard.mozilla.org/r/159544/#review165404
Attachment #8888567 - Flags: review?(mh+mozilla) → review+

Comment 40

7 months ago
mozreview-review
Comment on attachment 8888568 [details]
Bug 1382697 - Minor nsinstall.py code cleanup;

https://reviewboard.mozilla.org/r/159546/#review165406

::: commit-message-e0d25:9
(Diff revision 1)
> +* Removed redundant os.path import
> +* Reorder imports
> +* Delete some argument parsing code to screen for arguments not
> +  accepted by the Python version (the option parser will error on
> +  unknown arguments anyway)
> +* Remove some parents around a tuple assignment

parens
Attachment #8888568 - Flags: review?(mh+mozilla) → review+

Comment 41

7 months ago
mozreview-review
Comment on attachment 8888569 [details]
Bug 1382697 - Clean up I/O code in nsinstall.py;

https://reviewboard.mozilla.org/r/159548/#review165408
Attachment #8888569 - Flags: review?(mh+mozilla) → review+

Comment 42

7 months ago
mozreview-review
Comment on attachment 8888591 [details]
Bug 1382697 - Change chmod() logic;

https://reviewboard.mozilla.org/r/159572/#review165410

::: commit-message-ef76e:9
(Diff revision 1)
> +The redundant call in copy_all_entries() has been removed. The call
> +in handleTarget() has been moved ahead of copy_all_entries() to
> +ensure the target directory has proper permissions before
> +copy_all_entries() is called and that directory is operated on.

You should also mention that the call in copy_all_entries was redundant with the one from maybe_create_dir in the one call that doesn't come from handleTarget.
Attachment #8888591 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8888570 [details]
Bug 1382697 - Teach nsinstall.py to handle symlinks;

Looks like this is going to be reworked, so I'll review after that happens.
Attachment #8888570 - Flags: review?(mh+mozilla)
Comment on attachment 8888571 [details]
Bug 1382697 - Remove native nsinstall;

Unsetting review per comment 29.
Attachment #8888571 - Flags: review?(mh+mozilla)
(Assignee)

Comment 45

7 months ago
mozreview-review-reply
Comment on attachment 8888429 [details]
Bug 1382697 - Detect symlink and hard link support in configure;

https://reviewboard.mozilla.org/r/159370/#review165392

> why isn't this inline like for symlinks? In fact both functions are largely identical, you could write a template instead:
> 
> @template
> def check_sym_hard_links(sym_or_hard):
>     funcname = {
>         'symbolic': 'symlink',
>         'hard': 'link',
>     }[sym_or_hard]
>     @depends(check_build_environment)
>     @checking('for %s link support from topsrcdir to topobjdir' % sym_or_hard)
>     @imports(...)
>     def check(build_env):
>         func = getattr(os, funcname, None)
>         if not func:
>             return None
> 
>         source = ...
>         dest = ... '%slink.test' % sym_or_hard)
> 
>         try:
>             os.remove(dest)
>         ...
> 
>         try:
>             func(source, dest)
>         except OSError:
>             return None
>         finally:
>             os.remove(dest)
> 
>         return True
> 
> set_config('TOPOBJDIR_TOPSRCDIR_SYMLINKS', check_sym_hard_links('symbolic'))
> set_config('TOPOBJDIR_TOPSRCDIR_HARDLINKS', check_sym_hard_links('hard'))

This was mostly me not knowing how @template works. I figured it out and it will be in the next version.
(Assignee)

Comment 46

7 months ago
mozreview-review-reply
Comment on attachment 8888430 [details]
Bug 1382697 - Configure flag to control file installation mode;

https://reviewboard.mozilla.org/r/159372/#review165396

> While "install" in the build system has its terminology, install, in configure/make land usually is loaded and relates to `make install`, and this option name is confusing in that sense.
> 
> I'm also not entirely convinced we need to make this overridable.

I remember when we started agressively switching to install manifests and aggressively using symlinks that people were upset about changes of behavior. It made things like copying the objdir to different locations difficult. For these scenarios, I think it would be beneficial to allow developers to opt in to a non-symlink mode so they can produce a self-contained dist/ that they can copy around.

I agree the name of the option isn't great. But I can't come up with anything better :/
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 81

7 months ago
There's a bug floating around somewhere - likely in the jar.py refactor - because the latest Try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3ffe77775d54aa911fc7f7f65f8a6a3ddda560c is very red :/
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Attachment #8889008 - Attachment is obsolete: true
Attachment #8889008 - Flags: review?(mh+mozilla)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 111

7 months ago
And with the latest series I think I hit all the main pain points that were plaguing it.

jar.py is now using mozpack for I/O and fallout/bustage from 1st refactor earlier today should be cleared up.

config.mk now takes $(FILE_INSTALL_MODE) + $(NSDISTMODE) into account *before* launching nsinstall. This means the ugly `import buildconfig` hack in nsinstall.py is no longer required.

On my Linux machine, diffing a dist/ before and after this series only results in some relative symlinks being converted to absolute symlinks. That shouldn't be a big deal: we have plenty of absolute symlinks in dist/ already.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

7 months ago
Blocks: 1384271

Comment 144

7 months ago
mozreview-review
Comment on attachment 8889005 [details]
Bug 1382697 - Remove code to support JAR writing in jar.py;

https://reviewboard.mozilla.org/r/160044/#review169432

Less code! \o/
Attachment #8889005 - Flags: review?(mh+mozilla) → review+

Comment 145

7 months ago
mozreview-review
Comment on attachment 8889006 [details]
Bug 1382697 - Clean up I/O related code for jar.mn processing;

https://reviewboard.mozilla.org/r/160046/#review169434

::: python/mozbuild/mozbuild/jar.py:434
(Diff revision 4)
> -
> -            # open in binary mode, this can be images etc
>  
> -            inf = open(realsrc, 'rb')
> -            outf.write(inf.read())
> -            outf.close()
> +            with outHelper.getOutput(out) as ofh:
> +                with open(realsrc, 'rb') as ifh:
> +                    ofh.write(ifh.read())

shutil.copyfileobj?
Attachment #8889006 - Flags: review?(mh+mozilla) → review+

Comment 146

7 months ago
mozreview-review
Comment on attachment 8889007 [details]
Bug 1382697 - Use mozpack for I/O when processing jar.mn files;

https://reviewboard.mozilla.org/r/160048/#review169436

More less code \o/
Attachment #8889007 - Flags: review?(mh+mozilla) → review+

Comment 147

7 months ago
mozreview-review
Comment on attachment 8888429 [details]
Bug 1382697 - Detect symlink and hard link support in configure;

https://reviewboard.mozilla.org/r/159370/#review169438

::: build/moz.configure/init.configure:776
(Diff revision 9)
> +
> +    return check
> +
> +topobjdir_symlinks = check_links('symbolic', 'from topsrcdir to topobjdir',
> +                                 'mach')
> +set_config('TOPOBJDIR_TOPSRCDIR_SYMLINKS', topobjdir_symlinks)

s/TOP//g in the variable names would make this slightly better. Same in the check messages.
Attachment #8888429 - Flags: review?(mh+mozilla) → review+

Comment 148

7 months ago
mozreview-review
Comment on attachment 8888430 [details]
Bug 1382697 - Configure flag to control file installation mode;

https://reviewboard.mozilla.org/r/159372/#review169440

::: build/moz.configure/init.configure:783
(Diff revision 9)
>                                    'mach')
>  set_config('TOPOBJDIR_TOPSRCDIR_HARDLINKS', topobjdir_hardlinks)
>  set_config('TOPOBJDIR_INTRA_HARDLINKS',
>             check_links('hard', 'within topobjdir'))
>  
> +option('--with-file-install-mode',

maybe --with-file-copy-mode?

::: build/moz.configure/init.configure:784
(Diff revision 9)
>  set_config('TOPOBJDIR_TOPSRCDIR_HARDLINKS', topobjdir_hardlinks)
>  set_config('TOPOBJDIR_INTRA_HARDLINKS',
>             check_links('hard', 'within topobjdir'))
>  
> +option('--with-file-install-mode',
> +       nargs=1,

That is implied, no need to set it.

::: build/moz.configure/init.configure:786
(Diff revision 9)
> +       choices=('default', 'copy', 'symlink'),
> +       default='default',

You don't need the 'default' value. Just use choices=('copy', 'symlink'), and leave the default alone. Then you can check in the @depends function whether wanted.origin is 'default'.

::: build/moz.configure/init.configure:818
(Diff revision 9)
> +        elif hardlinks:
> +            return 'hardlink'
> +        else:
> +            return 'copy'
> +
> +    die('unhandled file install mode: %s' % wanted)

configure_error() when dealing with things that are not supposed to happen. die() is fine in the two other places.
Attachment #8888430 - Flags: review?(mh+mozilla)

Comment 149

7 months ago
mozreview-review
Comment on attachment 8889022 [details]
Bug 1382697 - Expand NSDISTMODE handling;

https://reviewboard.mozilla.org/r/160074/#review169446

::: config/config.mk:456
(Diff revision 3)
>  
>  else
>  
> -# This isn't laid out as conditional directives so that NSDISTMODE can be
> -# target-specific.
> -INSTALL         = $(if $(filter copy, $(NSDISTMODE)), $(NSINSTALL) -t, $(if $(filter absolute_symlink, $(NSDISTMODE)), $(NSINSTALL) -L $(PWD), $(NSINSTALL) -R))
> +ifeq ($(NSDISTMODE),copy)
> +INSTALL = $(NSINSTALL) -t
> +else ifeq ($(NSDISTMODE),absolute_symlink)

I'm not sure all makes actually support that form, and that would be the first use in the build system. I'd rather go with caution and just put the ifeq on a new line, and add the necessary endif.
Attachment #8889022 - Flags: review?(mh+mozilla) → review+

Comment 150

7 months ago
mozreview-review
Comment on attachment 8889023 [details]
Bug 1382697 - Remove support for NSDISTMODE==absolute_symlink;

https://reviewboard.mozilla.org/r/160076/#review169454

::: config/config.mk
(Diff revision 3)
>  
>  GARBAGE		+= $(DEPENDENCIES) core $(wildcard core.[0-9]*) $(wildcard *.err) $(wildcard *.pure) $(wildcard *_pure_*.o) Templates.DB
>  
> -ifeq ($(OS_ARCH),Darwin)
> -ifndef NSDISTMODE
> -NSDISTMODE=absolute_symlink

This is one of those rare cases where there *is* useful information in the miraculously referenced bug in the commit that introduced this 14 years ago:
https://bugzilla.mozilla.org/show_bug.cgi?id=193164#c4

And it looks like the problem might still apply across all the rsyncing that happens in browser/app/Makefile.in. That is, when copying relative symlinks from dist/bin to dist/Nightly.app/Contents/MacOS, they become broken symlinks.
Attachment #8889023 - Flags: review?(mh+mozilla) → review-

Comment 151

7 months ago
mozreview-review
Comment on attachment 8889024 [details]
Bug 1382697 - Honor file install mode for $(INSTALL);

https://reviewboard.mozilla.org/r/160078/#review169458

This depends on what happens with the r- patch.
Attachment #8889024 - Flags: review?(mh+mozilla)

Comment 152

7 months ago
mozreview-review
Comment on attachment 8888570 [details]
Bug 1382697 - Teach nsinstall.py to handle symlinks;

https://reviewboard.mozilla.org/r/159550/#review169462

::: config/nsinstall.py:34
(Diff revision 9)
>      p.add_option('-R', action="store_true",
> -                 help="Use relative symbolic links (ignored)")
> +                 help="Use symbolic links")

The option was -R for relative. -R for symlink is just weird, while -L is ignored...
Attachment #8888570 - Flags: review?(mh+mozilla)

Comment 153

7 months ago
mozreview-review
Comment on attachment 8888571 [details]
Bug 1382697 - Remove native nsinstall;

https://reviewboard.mozilla.org/r/159552/#review169464

::: config/Makefile.in
(Diff revision 9)
> -# L10n jobs are doing make -C config manually before anything else,
> -# and need nsinstall to be built as a consequence.

Note that depending how exactly the l10n jobs are calling into this file, this change might break them.
Attachment #8888571 - Flags: review?(mh+mozilla) → review+
(Assignee)

Updated

5 months ago
status-firefox57: --- → wontfix
You need to log in before you can comment on or make changes to this bug.