New issue
Advanced search Search tips

Issue 860761 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

compile (with patch) confirm no-op is failing

Project Member Reported by pdr@chromium.org, Jul 6

Issue description

The builders are failing due to the no-op build step:

https://logs.chromium.org/v/?s=chromium%2Fbuildbucket%2Fcr-buildbucket.appspot.com%2F8941732204902996016%2F%2B%2Fsteps%2Fcompile__with_patch__confirm_no-op%2F0%2Fstdout

ninja explain: output obj/third_party/blink/renderer/core/core_generated/css_unresolved_property.o older than most recent input gen/third_party/blink/renderer/core/css/properties/longhands/border_block_end_color.h (1530891556 vs 1530896863)
ninja explain: obj/third_party/blink/renderer/core/core_generated/css_unresolved_property.o is dirty
ninja explain: output obj/third_party/blink/renderer/core/css/css_4/border_block_end_color_custom.o older than most recent input gen/third_party/blink/renderer/core/css/properties/longhands/border_block_end_color.h (1530891570 vs 1530896863)
ninja explain: obj/third_party/blink/renderer/core/css/css_4/border_block_end_color_custom.o is dirty
ninja explain: output obj/third_party/blink/renderer/core/css/css_4/border_block_start_color_custom.o older than most recent input gen/third_party/blink/renderer/core/css/properties/longhands/border_block_start_color.h (1530891571 vs 1530896862)
ninja explain: obj/third_party/blink/renderer/core/css/css_4/border_block_start_color_custom.o is dirty
ninja explain: output obj/third_party/blink/renderer/core/css/css_4/border_inline_end_color_custom.o older than most recent input gen/third_party/blink/renderer/core/css/properties/longhands/border_inline_end_color.h (1530891570 vs 1530896862)
ninja explain: obj/third_party/blink/renderer/core/css/css_4/border_inline_end_color_custom.o is dirty
ninja explain: output obj/third_party/blink/renderer/core/css/css_4/border_inline_start_color_custom.o older than most recent input gen/third_party/blink/renderer/core/css/properties/longhands/border_inline_start_color.h (1530891570 vs 1530896862)
ninja explain: obj/third_party/blink/renderer/core/css/css_4/border_inline_start_color_custom.o is dirty
ninja explain: obj/third_party/blink/renderer/core/css/libcss_4.a is dirty
ninja explain: obj/third_party/blink/renderer/core/libcore_generated.a is dirty
ninja explain: obj/third_party/blink/renderer/core/css/libcss_4.a is dirty
ninja explain: v8_context_snapshot_generator is dirty
ninja explain: v8_context_snapshot.bin is dirty
ninja explain: obj/tools/v8_context_snapshot/generate_v8_context_snapshot.stamp is dirty
ninja explain: obj/third_party/blink/renderer/core/libcore_generated.a is dirty
ninja explain: obj/third_party/blink/renderer/core/css/libcss_4.a is dirty

I think something from https://crrev.com/572990 might have caused this and am going to try to roll out.
 
I rolled this patch out in https://crrev.com/573008.

@obrufau, you passed the CQ so this isn't your fault but we'll need to figure out what went wrong before re-landing. I wonder if the change to CSSProperties.json5 failed to invalidate some parts of the build? Normally, if you touch a .cc file, stuff gets rebuilt, but maybe touching CSSProperties.json5 doesn't? I don't know what needs to change to make this land cleanly.
> I wonder if the change to CSSProperties.json5 failed to invalidate some parts of the build?

Seems probable. First I compiled master, then I did my patch and recompiled, but the incremental build seemed to ignore the changes. I had to switch to another patch, compile, switch back to the :visited patch and compile again.
I see an entry in src/third_party/blink/renderer/core/BUILD.gn for runtime_enabled_features.json5 but no similar BUILD.gn file for CSSProperties.json5 (other than a devtools one). I wonder if that could be the root cause?
Possibly related: bug 860589

The problem was that
    valid_for_visited_link: true
in CSSProperties.json5 adds
    bool IsValidForVisitedLink() const override { return true; }
in out/Default/gen/third_party/blink/renderer/core/css/properties/longhands/border_block_start_color.h
But that file is not in BUILD.gn, because I didn't realize I was supposed to include the logical properties there when I implemented them. So the changes weren't noticed.

https://chromium-review.googlesource.com/c/chromium/src/+/1143761 should fix it.
Project Member

Comment 7 by bugdroid1@chromium.org, Aug 16

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

commit 45f3faa70f62e271f6a048a34e944258d4a62d87
Author: Oriol Brufau <obrufau@igalia.com>
Date: Thu Aug 16 02:00:13 2018

[css-logical] Include logical properties in blink/renderer/core/BUILD.gn

Otherwise, changes in CSSProperties.json5 may fail to invalidate parts
of the build. That's why https://crrev.com/572990 had to be reverted.

BUG= 860761 

Change-Id: I465fb8fa0970b908da2c2edd6d90efa20fe38b2b
Reviewed-on: https://chromium-review.googlesource.com/1143761
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Commit-Queue: Oriol Brufau <obrufau@igalia.com>
Cr-Commit-Position: refs/heads/master@{#583495}
[modify] https://crrev.com/45f3faa70f62e271f6a048a34e944258d4a62d87/third_party/blink/renderer/core/BUILD.gn

Status: Fixed (was: Assigned)
Patch for https://bugs.chromium.org/p/chromium/issues/detail?id=860313 has been relanded, now without problems.

Sign in to add a comment