1168231 - Normalize file mode in jars

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

When building with a different umask, jar creation ends up using different file modes.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Normalize file mode in jars

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8610489 [details] [diff] [review]
Normalize file mode in jars

Review of attachment 8610489 [details] [diff] [review]:
-----------------------------------------------------------------

::: python/mozbuild/mozpack/files.py
@@ +219,5 @@
> +        # - apply a standard umask
> +        # - keep file type (e.g. S_IFREG)
> +        file_type = stat.S_IFMT(mode)
> +        mode = mode & stat.S_IRWXU;
> +        mode = mode | (mode >> 3) | (mode >> 6)

Bit shifting is correct, but too clever for my liking. Please use S_IRWXG and S_IRWXO.

@@ +220,5 @@
> +        # - keep file type (e.g. S_IFREG)
> +        file_type = stat.S_IFMT(mode)
> +        mode = mode & stat.S_IRWXU;
> +        mode = mode | (mode >> 3) | (mode >> 6)
> +        return file_type | (mode & 0755);

It feels a bit weird to mix and match constants with the octal magic number. But it's file modes, so hopefully everyone groks that you are stripping group and other writable bits here.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #2)
> Comment on attachment 8610489 [details] [diff] [review]
> Normalize file mode in jars
> 
> Review of attachment 8610489 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: python/mozbuild/mozpack/files.py
> @@ +219,5 @@
> > +        # - apply a standard umask
> > +        # - keep file type (e.g. S_IFREG)
> > +        file_type = stat.S_IFMT(mode)
> > +        mode = mode & stat.S_IRWXU;
> > +        mode = mode | (mode >> 3) | (mode >> 6)
> 
> Bit shifting is correct, but too clever for my liking. Please use S_IRWXG
> and S_IRWXO.

S_IRWXG and S_IRWXO are resp. 070 and 007. They're not going to help doing what this line does, which is replicate the same mode bits from users to group and others.

> @@ +220,5 @@
> > +        # - keep file type (e.g. S_IFREG)
> > +        file_type = stat.S_IFMT(mode)
> > +        mode = mode & stat.S_IRWXU;
> > +        mode = mode | (mode >> 3) | (mode >> 6)
> > +        return file_type | (mode & 0755);
> 
> It feels a bit weird to mix and match constants with the octal magic number.
> But it's file modes, so hopefully everyone groks that you are stripping
> group and other writable bits here.

I thought stat.S_IRWXU | stat.S_IRGRP | stat.S_IXGRP | stat.S_IROTH | stat.S_IXOTH would be overkill. In fact, I find it less readable than 0755... in fact, come to think of it, I'd rather replace stat.S_IRWXU with 0700. Thoughts?

Comment 4

[Mike Hommey [:glandium]]

Or... that could be rewritten as:
 ret = file_type
 if (mode & stat.S_IRUSR)
     ret |= stat.S_IRUSR | stat.S_IRGRP | stat.S_IROTH
 if (mode & stat.S_IWUSR)
     ret |= stat.S_IWUSR
 if (mode & stat.S_IXUSR)
      ret |= stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH
 return ret

Comment 5

[Gregory Szorc [:gps]]

I like comment #4 because it constructs the final mode from scratch rather than creating something that's undone by a bit mask in the very last step. It's also clear that read and execute bits are global but write only applies to user. I would also accept this code with octal literals in it. e.g.

ret = file_type
# Read and execute privileges become global
if mode & 0400:
  ret |= 0444
if mode & 0100:
  ret |= 0111
# Write privileges only apply to owner
if mode & 0200:
  ret |= 0200

return ret

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55b6a6d3e2be

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/55b6a6d3e2be