Lint suppression configuration is a bit awkward
Reported by
ingem...@opera.com,
Jun 29 2017
|
|||
Issue descriptionLint 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..
,
Oct 2 2017
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.
,
Oct 3 2017
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?
,
Oct 3 2017
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.
,
Oct 3 2017
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.
,
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
,
Oct 4 2017
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.
,
Oct 4 2017
Reverting, thanks for detailed report!
,
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
,
Oct 5 2017
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.
,
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
,
Oct 13 2017
|
|||
►
Sign in to add a comment |
|||
Comment 1 by bugdroid1@chromium.org
, Jul 6 2017