New issue
Advanced search Search tips

Issue 750627 link

Starred by 1 user
Status: Duplicate
Merged: issue 748192
Owner: ----
Closed: Aug 2017
Cc:
qyears...@chromium.org
jeffcarp@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

[WPT import] Abort import attempt if there are patches (exportable commits) that don't apply

Project Member Reported by foolip@chromium.org, Jul 31 2017 
Running wpt-import manually today, I see this:

Cloning GitHub w3c/web-platform-tests into /tmp/wpt
No credentials given, using wpt mirror URL.
It is possible for the mirror to be delayed; see https://crbug.com/698272.
Noting the revision we are importing.
Patch did not apply cleanly for the following commit:
Message: Failed to run "['git', 'apply', '-']" exit_code: 1 cwd: /tmp/wpt

output: Last 500 characters of output:

error: mediacapture-fromelement/creation.html: patch does not apply
error: patch failed: mediacapture-fromelement/ended.html:1
error: mediacapture-fromelement/ended.html: patch does not apply
error: mediacapture-fromelement/webm/test-a-128k-44100Hz-1ch.webm: No such file or directory
error: mediacapture-fromelement/webm/test-av-384k-44100Hz-1ch-320x240-30fps-10kfr.webm: No such file or directory
error: mediacapture-fromelement/webm/test-v-128k-320x240-24fps-8kfr.webm: No such file or directory

If these are indeed errors, then the import should stop at this point. Instead it proceeds to create a commit with changes to WPT_BASE_MANIFEST.json only.

(Alternatively, these aren't actually errors, in which case only the log output needs to be changed.)

Comment 1 by foolip@chromium.org, Jul 31 2017

Cc: jeffcarp@chromium.org
Added some more of the log to the description. It points to https://chromium.googlesource.com/chromium/src/+/b684efb085 so I guess it's treating that as exportable. However, that's something we can't upstream because the thing it reverts wasn't landed upstream.

Jeff, is this commit still considered exportable by wpt-export as well? If not, should more of the logic for testing exportability be shared between import and export?

Comment 2 by qyears...@chromium.org, Jul 31 2017 

Depending on where that message occurred, I believe that this is the same as  bug 748192  -- the patch failure message is from the beginning part when it's checking if it's exportable; if the patch doesn't apply it's not exportable.

Even for a commit that was exported, it may be considered un-exportable on a later run if the patch no longer applies to wpt HEAD (maybe because it was already applied).

Comment 3 by robertma@chromium.org, Jul 31 2017 

If the logs were similar to this successful run (https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwpt-importer%2F400%2F%2B%2Frecipes%2Fsteps%2FImport_changes_from_WPT_to_Chromium%2F0%2Fstdout), i.e. the errors were logged pretty early in the process, then it can be safely ignored (and I'm starting to work on suppressing these logs).

> Even for a commit that was exported, it may be considered un-exportable on a later run if the patch no longer applies to wpt HEAD (maybe because it was already applied).

Wouldn't common.is_exportable look for the upstream PR for an exportable patche before testing the patch? If the PR already exists and is closed (merged/reverted), then the patch would be considered unexportable without being tested to apply against WPT head.

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

Mergedinto: 748192
Status: Duplicate (was: Untriaged)
What I pasted was from the beginning of the log, so duping into  issue 748192  as suggested.
►

Sign in to add a comment