Closed Bug 1382739 Opened 7 years ago Closed 7 years ago

Add hardlink support to mozbuild

Categories

(Firefox Build System :: General, enhancement)

enhancement
Not set
normal

Tracking

(firefox56 fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: Alex_Gaynor, Assigned: Alex_Gaynor)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Right now mozbuild knows how to copy, and how to symlink. We should teach it to hardlink as well, for usage in 1380416.
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165356

This is essentially an r+. Just need some minor test fixups.

::: python/mozbuild/mozpack/files.py:442
(Diff revision 1)
> +        else:
> +            return True

Nit: you can move this into the try: block.

::: python/mozbuild/mozpack/test/test_files.py:422
(Diff revision 1)
> +        if not self.hardlink_supported:
> +            return

IIRC the better way to do this is: `raise unittest.SkipTest('hardlink not supported')`

::: python/mozbuild/mozpack/test/test_files.py:453
(Diff revision 1)
> +        s = HardlinkFile(source)
> +        self.assertFalse(s.copy(dest))
> +
> +        source_stat = os.stat(source)
> +        dest_stat = os.stat(dest)
> +        self.assertEqual(source_stat.st_dev, dest_stat.st_dev)
> +        self.assertEqual(source_stat.st_ino, dest_stat.st_ino)

It feels like this test should lstat() the previous link and make sure it isn't updated as part of calling `s.copy()`. I'm not sure what attributes are available on hard links though. Maybe you could os.utime() the link to a very old time?
Attachment #8888433 - Flags: review?(gps) → review-
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165356

> It feels like this test should lstat() the previous link and make sure it isn't updated as part of calling `s.copy()`. I'm not sure what attributes are available on hard links though. Maybe you could os.utime() the link to a very old time?

Not sure I understand this -- do hardlinks keep separate metadata from their target? I switched the test to use `lstat` so that if ther was a symlink somehow the test would fail.
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165356

> Not sure I understand this -- do hardlinks keep separate metadata from their target? I switched the test to use `lstat` so that if ther was a symlink somehow the test would fail.

It appears that they do not. So the best we can do is compare st_nlink and st_ino for the proper semantics.
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165356

> It appears that they do not. So the best we can do is compare st_nlink and st_ino for the proper semantics.

Just so I follow, you'd like a `self.assertEqual(source_stat.st_nlink, dest_stat.st_link)` as well?
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165794

I guess you can change those lstat() back to stat() if you want. Sorry for misleading you. I always thought lstat() on a hard link yielded something more useful.
Attachment #8888433 - Flags: review?(gps) → review+
Comment on attachment 8888433 [details]
Bug 1382739 - Added support for hardlinks to mozbuild;

https://reviewboard.mozilla.org/r/159380/#review165794

`lstat` still makes more sense I think, so I'll leave it.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9efa1cfe64b1
Added support for hardlinks to mozbuild; r=gps
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9efa1cfe64b1
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.