Closed
Bug 1168231
Opened 9 years ago
Closed 9 years ago
Normalize file mode in jars
Categories
(Firefox Build System :: General, defect)
Firefox Build System
General
Tracking
(firefox41 fixed)
RESOLVED
FIXED
mozilla41
Tracking | Status | |
---|---|---|
firefox41 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
1.53 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
When building with a different umask, jar creation ends up using different file modes.
Assignee | ||
Comment 1•9 years ago
|
||
Assignee: nobody → mh+mozilla
Attachment #8610489 -
Flags: review?(gps)
Comment 2•9 years ago
|
||
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.
Attachment #8610489 -
Flags: review?(gps) → review+
Assignee | ||
Comment 3•9 years ago
|
||
(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?
Assignee | ||
Comment 4•9 years ago
|
||
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•9 years ago
|
||
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
https://hg.mozilla.org/mozilla-central/rev/55b6a6d3e2be
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox41:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Updated•6 years ago
|
Product: Core → Firefox Build System
You need to log in
before you can comment on or make changes to this bug.
Description
•