New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 754616 link

Starred by 1 user

Issue metadata

Status: Duplicate
Merged: issue 743149
Owner: ----
Closed: Aug 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

[WPT Export] Exporter unable to create PR due to conflicts, but nothing goes red

Project Member Reported by foolip@chromium.org, Aug 11 2017

Issue description

https://chromium-review.googlesource.com/608629 depends on https://chromium-review.googlesource.com/602381, and trying to apply it on wpt isn't possible before https://github.com/w3c/web-platform-tests/pull/6759 is merged.

This is from the logs of https://build.chromium.org/p/chromium.infra.cron/builders/wpt-exporter/builds/32564:

The following commit did not apply cleanly:
Subject: Parse ranges for font-style in @font-face (https://chromium.googlesource.com/chromium/src/+/8b4edde07c)
Failed to run "['git', 'apply', '-']" exit_code: 1 cwd: /tmp/wpt
output: error: patch failed: css-fonts/variations/font-parse-numeric-stretch-style-weight.html:90
error: css-fonts/variations/font-parse-numeric-stretch-style-weight.html: patch does not apply
The following commit did not apply cleanly:
Subject: Allow Date.now() rounding errors in timeOrigin test (https://chromium.googlesource.com/chromium/src/+/ed96173eaf)
Failed to run "['git', 'apply', '-']" exit_code: 1 cwd: /tmp/wpt
output: error: patch failed: hr-time/timeOrigin.html:10
error: hr-time/timeOrigin.html: patch does not apply

https://chromium.googlesource.com/chromium/src/+/8b4edde07c landed on Aug 4 and we haven't noticed until now, and wouldn't have it not for the other conflicting change.

If left alone, this would eventually lead to a revert.

When it's not possible to create PRs due to conflicts, something needs to go red so that it is noticed by the Ecosystem Infra rotation.
 

Comment 1 by foolip@chromium.org, Aug 11 2017

Labels: -Pri-3 Pri-1

Comment 2 by foolip@chromium.org, Aug 11 2017

Cc: qyears...@chromium.org raphael....@intel.com
It looks the importer already did revert changes from https://chromium.googlesource.com/chromium/src/+/8b4edde07c in two different CLs:
https://chromium-review.googlesource.com/602030 (manual by rakuco)
https://chromium-review.googlesource.com/602471 (automatic)

Comment 3 by foolip@chromium.org, Aug 11 2017

One additional thing to worry about:

https://chromium.googlesource.com/chromium/src/+/ed96173eaf is now exported as part of https://github.com/w3c/web-platform-tests/pull/6822, and I maintained the two commits separately. The exporter should therefore find these and not try to export them again. However, I could have squashed them into two, deliberate or my accident. Then, the exporter would keep trying to export ed96173eaf, and if at some point in the future it stops conflicting, it would be merged. That would be very surprising.

Comment 4 by foolip@chromium.org, Aug 11 2017

After the manual fixup in https://github.com/w3c/web-platform-tests/pull/6845, https://chromium.googlesource.com/chromium/src/+/8b4edde07c no longer conflicted when applied on wpt, and https://github.com/w3c/web-platform-tests/pull/6846 was automatically created, probably merged soon.

For the timeOrigin changes, the most recent export logs still complain that "Allow Date.now() rounding errors in timeOrigin test" can't be applied cleanly, meaning that it's trying to export it.

I think that's because the exporter expects each chromium change to go in one PR, but I had to put two commits in a single PR to get stable results and merge them. The exporter will keep trying to create a PR for that change until it goes out of the 5000 commit window. A fix for this would be to first look in the wpt commit history to determine what's been exported, and then look for PRs only for the things that remain.
Mergedinto: 743149
Status: Duplicate (was: Untriaged)
First of all, this issue is a dup of  issue 743149 . Marking as so.

Some more information regarding the timeOrigin test:

> I maintained the two commits separately. The exporter should therefore find these and not try to export them again. However, I could have squashed them into two, deliberate or my accident. Then, the exporter would keep trying to export ed96173eaf, and if at some point in the future it stops conflicting, it would be merged. That would be very surprising.

Unfortunately this is not true. The exporter does not look at GitHub commits, but rather PRs. It would normally expect the two changes to go in two separate PRs, with the change IDs in description. Now that we have one PR, the remediation is to put the two change IDs into the PR description (and the exporter will link this PR with both changes), if I understand correctly. foolip@ has already modified the PR description in https://github.com/w3c/web-platform-tests/pull/6822.
> Now that we have one PR, the remediation is to put the two change IDs into the PR description (and the exporter will link this PR with both changes), if I understand correctly. foolip@ has already modified the PR description in https://github.com/w3c/web-platform-tests/pull/6822.

Unfortunately again, this remediation doesn't work -- the exporter only looks at the first "Change-Id". But it wouldn't cause more trouble than an error message in the logs, which looks like:

The following commit did not apply cleanly:
Subject: Allow Date.now() rounding errors in timeOrigin test (https://chromium.googlesource.com/chromium/src/+/ed96173eaf)

The error message will eventually go away after this commit goes out of the history window.

Sign in to add a comment