Closed Bug 1376652 Opened 3 years ago Closed 3 years ago

Include security/sandbox/chromium-shim/ in ThirdPartyPaths.txt

Categories

(Firefox Build System :: Source Code Analysis, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED WONTFIX

People

(Reporter: janx, Assigned: janx)

References

Details

Attachments

(1 file)

Bug 1102215 moved security/sandbox/chromium/base/shim/ out to security/sandbox/chromium-shim/, and ThirdPartyPaths.txt should probably be updated.
No longer blocks: 1102215
Depends on: 1102215
Attachment #8881636 - Flags: review?(ted)
Attachment #8881636 - Flags: review?(sledru)
Are you sure? 
seems like code that we modified / own
(Please use needinfo, otherwise I'll probably miss questions)

(In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> Are you sure? 
> seems like code that we modified / own

It does look like something we forked, but for example the top comment mentions that the coding style is kept intentionally close to Chromium's in order to facilitate future merges (so we probably shouldn't clang-format it to Mozilla style).

Additionally, the entire folder used to live under security/sandbox/chromium/, so it was covered by ThirdPartyPaths.txt originally, and this changed (maybe unintentionally) when it was moved out.

Ted, any thoughts on ignoring security/sandbox/chromium-shim/ while doing static analysis fixes or other automated rewriting?
Flags: needinfo?(ted)
I'm afraid I don't know whether that code is considered third-party or not! Bob: can you answer Jan's question in comment 3?
Flags: needinfo?(ted) → needinfo?(bobowencode)
(In reply to Jan Keromnes [:janx] from comment #3)
> (Please use needinfo, otherwise I'll probably miss questions)
> 
> (In reply to Sylvestre Ledru [:sylvestre] from comment #2)
> > Are you sure? 
> > seems like code that we modified / own
> 
> It does look like something we forked, but for example the top comment
> mentions that the coding style is kept intentionally close to Chromium's in
> order to facilitate future merges (so we probably shouldn't clang-format it
> to Mozilla style).
> 
> Additionally, the entire folder used to live under
> security/sandbox/chromium/, so it was covered by ThirdPartyPaths.txt
> originally, and this changed (maybe unintentionally) when it was moved out.
> 
> Ted, any thoughts on ignoring security/sandbox/chromium-shim/ while doing
> static analysis fixes or other automated rewriting?

This is our code, so we should probably be running static analysis on it, but we don't really want to do any automated rewriting.
Flags: needinfo?(bobowencode)
(In reply to Bob Owen (:bobowen) from comment #5)
> This is our code, so we should probably be running static analysis on it,
> but we don't really want to do any automated rewriting.

Sylvestre, is ThirdPartyPaths.txt a good place for folders where we want to run static analysis, but not do any automated rewriting in?

If it's not clear what to do with this folder, I'll opt for doing nothing and close this bug as WONTFIX.
Flags: needinfo?(sledru)
To me, we want to run static analysis on all codes (including thirdpartypaths.txt libraries) as third party bugs might impact Firefox at the end.

Bug we don't want to run coding style tools (eslint, clang-format, etc) on them.

As it is our code, it should not be on that list and we should wontfix it.
Flags: needinfo?(sledru)
Attachment #8881636 - Flags: review?(sledru) → review-
Works for me. Thanks!
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WONTFIX
Attachment #8881636 - Flags: review?(ted)
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.