New issue
Advanced search Search tips

Issue 737897 link

Starred by 4 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 2017
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 3
Type: Bug



Sign in to add a comment

Lint suppression configuration is a bit awkward

Reported by ingem...@opera.com, Jun 29 2017

Issue description

Lint suppression configuration is done "globally" for all targets, where suppressions are written to //build/android/lint/suppressions.xml. There is a //build/android/lint/suppress.py utility to help with managing the suppression configuration.

//build_overrides/build.gni can specify a 'lint_suppressions_file' to override the otherwise hard-coded path to //build/android/lint/suppressions.xml, but this is a bit awkward.

It would be nice if each target which has "lint capability" (currently 'java_library_impl' targets with supports_android && chromium_code) could specify it's own suppression configuration file. It could however get complicated when dependencies has their own suppression configuration, which has to be merged first..
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jul 6 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0972c74b68f73edc01a5553243ca26b8c723a9e2

commit 0972c74b68f73edc01a5553243ca26b8c723a9e2
Author: Ingemar Ådahl <ingemara@opera.com>
Date: Thu Jul 06 16:20:37 2017

Add command line flag to override suppression.xml path

Let the suppression script optionally write to a configuration file set
on the command line, rather than the default.

//build_overrides/build.gni can set a 'lint_suppressions_file' variable,
allowing different products to override the lint suppression
configuration. Updating this overriding suppression configuration can
otherwise be cumbersome.

Bug:  737897 
Change-Id: I1551d7e6951f212e42ee79ffac568e9a5208aec9
Reviewed-on: https://chromium-review.googlesource.com/561137
Reviewed-by: Peter Wen <wnwen@chromium.org>
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Ingemar Ådahl <ingemara@opera.com>
Cr-Commit-Position: refs/heads/master@{#484621}
[modify] https://crrev.com/0972c74b68f73edc01a5553243ca26b8c723a9e2/build/android/lint/suppress.py

As another Chromium embedder, we would also benefit from being able to define a suppression file on a per-target basis (assuming said target is "lint capable").

I have looked into merging suppression files as GN goes up the dependency tree. However, I haven't been able to find a good GN construct where suppression configuration filepaths can be passed up the dependency chain. I saw the predefined target variable "public_config" but it is not propagated up the tree unless the dependency is listed a public.

Would upstream be willing to accept a change where you can define a list of "global" suppression files rather than just one? This would alleviate the need for per-target suppression configurations (using regex in supressions.xml should be good enough to separate Chromium and embedder code), while also allowing embedders to maintain their own suppression file without directly editing Chromium's suppression file.
I certainly agree having a global suppression file is not the best. I don't think inheriting suppressions from your deps is really desirable though. E.g. A dep might turn off a check globally, but your target may want that check enabled.

Perhaps each target should accept a list of suppression.xml files directly?
Thanks for the quick response!

After looking further into the Android lint documentation, I agree, it doesn't look like merging and inheriting suppression rules while going up the dependency tree is necessary. I assumed merging suppression files might be necessary in the case that an Android lint check was configured to detect the same issue type in both java source files AND class files (since both are passed to the linter)...However, it looks like my assumptions were incorrect.

That being said, what are your thoughts on allowing each target to define a single suppression.xml file (simple ~4 line change), rather than accepting a list of files and combining them? I guess I'm not clear on the advantage of defining multiple suppression files for a given target.
If that works for you, then I'd be happy to have that added in. I was guessing you'd want multiple so that you'd have [global_suppressions.xml, target_suppressions.xml]. 

That could always be added in later though, if a single override is not sufficient.
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1dd22ad042686bbf63b2f89efb466528cc682ffe

commit 1dd22ad042686bbf63b2f89efb466528cc682ffe
Author: Justin Okamoto <justmoto@amazon.com>
Date: Wed Oct 04 15:16:32 2017

Android: Allow per-target lint suppression configuration

This change allows for Android lint suppression configurations
to be defined on a per-target basis (assuming said target is
"Android lint capable") rather than having one global config.
This will allow embedders to keep upstream's lint config for
Chromium targets (w/o patching), while using a separate config
for the embedding application.

Bug:  737897 
Change-Id: Ieccb95767e0ce541522d7a53edbe734c5bbfe6dc
Reviewed-on: https://chromium-review.googlesource.com/698297
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506393}
[modify] https://crrev.com/1dd22ad042686bbf63b2f89efb466528cc682ffe/AUTHORS
[modify] https://crrev.com/1dd22ad042686bbf63b2f89efb466528cc682ffe/build/config/android/internal_rules.gni

Hi,
the change linked above breaks WebRTC (but this is not an emergency because we don't roll in the change if it's broken)

https://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/28406/steps/generate_build_files/logs/stdio

ERROR at //build/config/android/internal_rules.gni:2715:45: Clobbering existing value.
          forward_variables_from(invoker, [ "lint_suppressions_file" ])
                                            ^-----------------------
The current scope already defines a value "lint_suppressions_file".
forward_variables_from() won't clobber existing values. If you want to
merge lists, you'll need to do this explicitly.
See //build_overrides/build.gni:31:26: value being clobbered.
lint_suppressions_file = "//tools_webrtc/android/suppressions.xml"
                         ^----------------------------------------
See //build/config/android/rules.gni:1544:5: whence it was called.
    java_library_impl(target_name) {
    ^-------------------------------
See //examples/BUILD.gn:70:3: whence it was called.
  android_library("AppRTCMobile_javalib") {
  ^----------------------------------------
See //BUILD.gn:23:7: which caused the file to be included.
      "examples",
      ^---------


I don't think a normal use of the build_overrides file is supposed to break like this.
Please revert or show how we can work around this.
Status: Started (was: Untriaged)
Reverting, thanks for detailed report!
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 4 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2898491636d1bfc1bfddee39f07c7b8a6e1c6eb8

commit 2898491636d1bfc1bfddee39f07c7b8a6e1c6eb8
Author: Andrew Grieve <agrieve@chromium.org>
Date: Wed Oct 04 19:17:02 2017

Revert "Android: Allow per-target lint suppression configuration"

This reverts commit 1dd22ad042686bbf63b2f89efb466528cc682ffe.

Reason for revert: 
Breaks when lint_suppressions_file is assigned within
//build_overrides/build.gni

Original change's description:
> Android: Allow per-target lint suppression configuration
> 
> This change allows for Android lint suppression configurations
> to be defined on a per-target basis (assuming said target is
> "Android lint capable") rather than having one global config.
> This will allow embedders to keep upstream's lint config for
> Chromium targets (w/o patching), while using a separate config
> for the embedding application.
> 
> Bug:  737897 
> Change-Id: Ieccb95767e0ce541522d7a53edbe734c5bbfe6dc
> Reviewed-on: https://chromium-review.googlesource.com/698297
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#506393}

TBR=agrieve@chromium.org,justmoto@amazon.com

Change-Id: Ib15a61731179cc9ffc8d415844cfe8fbbc8e68eb
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  737897 
Reviewed-on: https://chromium-review.googlesource.com/701014
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506475}
[modify] https://crrev.com/2898491636d1bfc1bfddee39f07c7b8a6e1c6eb8/AUTHORS
[modify] https://crrev.com/2898491636d1bfc1bfddee39f07c7b8a6e1c6eb8/build/config/android/internal_rules.gni

My apologies! I fixed the scoping issue that broke the build using build overrides to set the lint configuration file and submitted a CR 701317.
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 5 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/78485ef14d15b0b51d6f49d7231f44a343bd468f

commit 78485ef14d15b0b51d6f49d7231f44a343bd468f
Author: Justin Okamoto <justmoto@amazon.com>
Date: Thu Oct 05 21:20:20 2017

Reland "Android: Allow per-target lint suppression configuration"

This is a reland of 1dd22ad042686bbf63b2f89efb466528cc682ffe
Original change's description:
> Android: Allow per-target lint suppression configuration
> 
> This change allows for Android lint suppression configurations
> to be defined on a per-target basis (assuming said target is
> "Android lint capable") rather than having one global config.
> This will allow embedders to keep upstream's lint config for
> Chromium targets (w/o patching), while using a separate config
> for the embedding application.
> 
> Bug:  737897 
> Change-Id: Ieccb95767e0ce541522d7a53edbe734c5bbfe6dc
> Reviewed-on: https://chromium-review.googlesource.com/698297
> Reviewed-by: Andrew Grieve <agrieve@chromium.org>
> Commit-Queue: Andrew Grieve <agrieve@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#506393}

Bug:  737897 
Change-Id: I4d1aecea7d4893f8f525d53a13b4044f27a17221
Reviewed-on: https://chromium-review.googlesource.com/701317
Reviewed-by: Andrew Grieve <agrieve@chromium.org>
Commit-Queue: Andrew Grieve <agrieve@chromium.org>
Cr-Commit-Position: refs/heads/master@{#506862}
[modify] https://crrev.com/78485ef14d15b0b51d6f49d7231f44a343bd468f/AUTHORS
[modify] https://crrev.com/78485ef14d15b0b51d6f49d7231f44a343bd468f/build/config/android/internal_rules.gni

Status: Fixed (was: Started)

Sign in to add a comment