Open Bug 1382697 Opened 7 years ago Updated 2 years ago

Detect filesystem and link support in configure

Categories

(Firefox Build System :: General, enhancement)

Product:
Firefox Build System
Component:
General
Type:
enhancement

Tracking

(firefox57 wontfix)

Tracking Flags:
Tracking Status
firefox57 --- wontfix

People

(Reporter: gps, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(19 files, 1 obsolete file)

Bug 1382697 - Use non-mutable default argument;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Detect symlink and hard link support in configure;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Configure flag to control file installation mode;
59 bytes, text/x-review-board-request
Details
Bug 1382697 - Remove --no-symlinks argument from process_install_manifest;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Honor file install mode when processing install manifests;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Handle pattern symlinks in InstallManifestNoSymlinks;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Honor file install mode when processing jars;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Reformat nsinstall.py;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Minor nsinstall.py code cleanup;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Clean up I/O code in nsinstall.py;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Teach nsinstall.py to handle symlinks;
59 bytes, text/x-review-board-request
Details
Bug 1382697 - Remove native nsinstall;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Change chmod() logic;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Remove code to support JAR writing in jar.py;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Clean up I/O related code for jar.mn processing;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Use mozpack for I/O when processing jar.mn files;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Expand NSDISTMODE handling;
59 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1382697 - Remove support for NSDISTMODE==absolute_symlink;
59 bytes, text/x-review-board-request
glandium
: review-
Details
Bug 1382697 - Honor file install mode for $(INSTALL);
59 bytes, text/x-review-board-request
Details 
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 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 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.
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 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 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 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 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 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 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 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 on attachment 8888567 [details]
Bug 1382697 - Reformat nsinstall.py;

https://reviewboard.mozilla.org/r/159544/#review165404
Attachment #8888567 - Flags: review?(mh+mozilla) → 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 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 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)
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.
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 :/
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 :/
Attachment #8889008 - Attachment is obsolete: true
Attachment #8889008 - Flags: review?(mh+mozilla)
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.
Blocks: 1384271
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 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 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 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 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 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 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 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 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 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+
status-firefox57: --- → wontfix
Product: Core → Firefox Build System
I'm not actively working on this. Most of these patches should still be trivially rebasable and landable, however.

https://hg.mozilla.org/users/gszorc_mozilla.com/firefox/rev/5275a9705243 is the latest version of this stack in my repo if someone wants to pull it.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: